Improve support of sub/superscript #755
Conversation
|
Build succeeded. |
| textDecorationStyle += ' double'; | ||
| break; | ||
| case 'single': | ||
| textDecorationStyle += ' double'; |
|
Build succeeded. |
|
This works fine, and after one pass through the patch, the code also looks good. But I'd suggest splitting up the strikethrough and the font size commits into two separate PRs... with getComputedStyle tests for the double strikethroughs. |
|
Build succeeded. |
|
I've pulled the strikethrough stuff out and will push it up, but (as was hinted by the lack of tests for that specific feature) no tests are possible for that part. getComputedStyle only returns style information it understands, which differs from browser to browser. I can write a layouttest that passes in FF, or one that passes in Chrome (not that I can check anything in Chrome), but not one that passes in both. |
peitschie
changed the title
|
Without a layout test possible, would it make sense to add this to visual-tests.odt, with some png as reference? Something like " |
|
@kossebau except, it can't be visually detected in browsers other than FF either :(. Note, I didn't make strikethrough work, that already did. All I did was make double-strikethrough work on FF only (and things supporting the new CSS3 I split that out a into new PR either way, so #758 is the correct place for further discussion on that :-). |
|
(strike was only taken as example because I knew a markdown notation to have it rendered) |
| * @return {!{verticalTextPosition: !string, fontHeight: (!string|undefined)}} | ||
| */ | ||
| function parseTextPosition(position) { | ||
| var parts = position.split(/\s/g); |
There was a problem hiding this comment.
I could not find a definition of what "white space separated" exactly means in "The style:text-position attribute has one or two white space separated values.". So for now I assume in my superficial knowledge it could mean that first better collapsing is to be done on the whitespaces in the list, if it is to be a list value as specified by XML (cmp 2.5.1.2 List datatypes, where the constraining facets contains "whiteSpace" with a fixed value of "collapse". http://www.w3.org/TR/xmlschema-2/#atomic-vs-list).
There was a problem hiding this comment.
Done. I've extracted this out into odf.StyleParseUtils and even provided unit tests!
|
Build succeeded. |
| } | ||
| // Calling split on an empty string returns a [""]. Avoid this by only attempting to split if the | ||
| // string is non-zero-length | ||
| return text && text.length > 0 ? text.split(/\s/) : []; |
There was a problem hiding this comment.
Curious: I guess you did replace(/\s+/g, " ") above for a reason instead of text.split(/\s+/), right? What is the catch with that?
There was a problem hiding this comment.
Contrary to my initial expectations, text.split(/\s+/) creates a bunch of empty strings in the array as well, rather than consuming multiple spaces as one separator :-/
There was a problem hiding this comment.
So could you please add a note about that (and mention where it fails), to prevent somebody "improving" this? E.g. on my Firefox and Chromium it works as expected:
" a b ".replace(/^\s*(.*?)\s*$/g, "$1").split(/\s+/)
["a", "b"]
There was a problem hiding this comment.
Can do... it isn't completely vital though. If someone "improves" this and it breaks, the unit tests will fail, hence informing said developer that their improvement was a failure. I don't trust to comments what I can enforce with failing tests...
There was a problem hiding this comment.
(well, unit tests do not fail for me on this "improvement") 🏄
There was a problem hiding this comment.
Have confirmed here, so I will choose to avoid the extra doco overhead that would simply record I was partially blind at the time of concocting this reason, and instead update the code ^_^
|
Build succeeded. |
| @@ -2,6 +2,8 @@ | |||
|
|
|||
| ## WebODF | |||
|
|
|||
| * Add support for subscript & superscript ([#755](https://github.com/kogmbh/WebODF/pull/755)) | |||
There was a problem hiding this comment.
Please also add Section title ### Improvements
|
Otherwise fine for shipment! :) |
style:text-position defines sub and super-script text properties.
ODT files are generally created in this directory as a result of running the layout tests from the source directory.
|
Build succeeded. |
Improve support of sub/superscript
Test document: https://drive.google.com/file/d/0B1EH5OPb-RrjbW9DQ1VRLUxQUEE/edit?usp=sharing