Bug 91660

Created attachment 153075 [details] Patch

Attachment 153075 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/rendering/style/StyleInheritedData.h:58: specified_line_height is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 153075 [details] Patch Attachment 153075 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13277639

Comment on attachment 153075 [details] Patch Attachment 153075 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13278649 New failing tests: fast/css/getComputedStyle/computed-style-with-zoom.html printing/page-rule-selection.html media/video-zoom-controls.html svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm

Created attachment 153093 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 153075 [details] Patch Attachment 153075 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13269686 New failing tests: fast/css/getComputedStyle/computed-style-with-zoom.html printing/page-rule-selection.html media/video-zoom-controls.html svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm

Created attachment 153114 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

I notice 3 of the bots are red. I'm happy to review this patch, but it's probably best to start with a green-bots patch. :)

Comment on attachment 153075 [details] Patch So we need access to both the original line height and the multiplied line height? It feels odd to hang this off of RenderStyle, instead of doing the multiplication at the locations its used. But I will need to look at the patch more carefully to understand your constraints.

Comment on attachment 153075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153075&action=review > Source/WTF/ChangeLog:6 > + Adds an almostEquals function for testing float equality. This uses a fuzzyCompare? > Source/WebCore/rendering/TextAutosizer.cpp:93 > + > + RefPtr<RenderStyle> newParentStyle = RenderStyle::clone(text->parent()->style()); Maybe add the comment above like // Increase computed line height pro... > Source/WebCore/rendering/style/RenderStyle.cpp:1463 > +void RenderStyle::setLineHeight(Length v) > +{ > +#if ENABLE(TEXT_AUTOSIZING) > + setSpecifiedLineHeight(v); > + float multiplier = textSizeMultiplier(); > + if (multiplier != 1) > + TextAutosizer::adjustLineHeight(this, multiplier); > + else > + setComputedLineHeight(v); > +#else > + SET_VAR(inherited, line_height, v) > +#endif > +} Why not just let adjustLineHeight return the value and set v to it in that case, and then end up calling SET_VAR? Then you can also get rid of the confusing setCompuledLineHeight.

Comment on attachment 153075 [details] Patch John, would you be willing to revise your patch so that it compiles and passes the style-bot?

Your patch also seems to be causing a few tests to fail, which might be a good thing to address in an updated version of the patch.

Created attachment 155371 [details] Patch

(In reply to comment #9) > (From update of attachment 153075 [details]) > So we need access to both the original line height and the multiplied line height? It feels odd to hang this off of RenderStyle, instead of doing the multiplication at the locations its used. But I will need to look at the patch more carefully to understand your constraints. I've been thinking about this, and it does seem like I'm overcomplicating things. The old text zoom code path managed fine with just one line height, which gets multiplied by the text zoom in StyleBuilder.cpp's ApplyPropertyLineHeight. Hence RenderStyle only knows about the scaled line height, and if the text zoom factor changes then a recalcStyle just causes ApplyPropertyLineHeight to recompute the line height. This sounded cleaner, so I've spent some time learning how that works and refactored the patch to use a similar approach. Does this sound like the right way to do things? I've just posted a patch showing the rough approach I'm aiming towards. It's still very much a work in progress (not ready for review, though I wouldn't mind getting some hints on how to improve it!). I've marked the main known issues with FIXMEs in the code. Three of the added/modified tests don't yet work in this refactored patch: - fast/text-autosizing/simple-paragraph.html: Text fails to increase in size. - fast/text-autosizing/narrow-child.html: First & third paragraphs fail to increase in size. - fast/text-autosizing/span-child.html: Text fails to increase in size. On more complicated pages like wikipedia text usually does end up the right size, but the line heights get multiplied several times, especially with nested elements (so they end up being the square or the cube of the textSizeMultiplier), presumably because the code added to StyleResolver::applyProperty isn't properly distinguishing inherited line height. I'll keep iterating... (In reply to comment #6) > (From update of attachment 153075 [details]) > Attachment 153075 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/13269686 > > New failing tests: > fast/css/getComputedStyle/computed-style-with-zoom.html > printing/page-rule-selection.html > media/video-zoom-controls.html > svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm The computed-style-with-zoom test was failing because I was relying on computedSize / specifiedSize giving the textSizeMultiplier, but computedSize can also be affected by zoom. I think some of the other failures are similar. The refactored patch probably isn't susceptible to this. (In reply to comment #2) > Source/WebCore/rendering/style/StyleInheritedData.h:58: specified_line_height is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] I did this for consistency with the surrounding code, which uses underscores. I've now removed those lines anyway, but for future reference should I have renamed those variables to not to contain underscores either? (In reply to comment #10) > (From update of attachment 153075 [details]) > ... > fuzzyCompare? > ... > Maybe add the comment above like // Increase computed line height pro... > ... > Why not just let adjustLineHeight return the value and set v to it in that case, and then end up calling SET_VAR? Then you can also get rid of the confusing setCompuledLineHeight. Thanks for the suggestions, they all sound good; unfortunately those lines all got removed in the refactor.

Created attachment 156749 [details] Patch Getting closer. Still work in progress, but I'd like feedback on whether this is the right general approach. I've fixed the issues with those 3 layout tests. I've also added a new layout test which is a reduced testcase from Wikipedia, and fixed one issue with that, though one issue (with nested inline elements) remains.

Comment on attachment 156749 [details] Patch Attachment 156749 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13450240 New failing tests: canvas/philip/tests/2d.text.draw.align.end.rtl.html canvas/philip/tests/2d.text.draw.align.right.html canvas/philip/tests/2d.text.draw.fill.maxWidth.small.html canvas/philip/tests/2d.missingargs.html canvas/philip/tests/2d.state.saverestore.font.html canvas/philip/tests/2d.text.measure.width.empty.html canvas/philip/tests/2d.text.draw.align.center.html canvas/philip/tests/2d.text.draw.kern.consistent.html canvas/philip/tests/2d.text.draw.align.left.html canvas/philip/tests/2d.text.draw.stroke.basic.html canvas/philip/tests/2d.text.draw.align.start.rtl.html canvas/philip/tests/2d.text.draw.fill.maxWidth.bound.html canvas/philip/tests/2d.text.draw.align.end.ltr.html canvas/philip/tests/2d.text.draw.fontface.html canvas/philip/tests/2d.text-custom-font-load-crash.html canvas/philip/tests/2d.text.draw.fill.unaffected.html canvas/philip/tests/2d.text.draw.fill.maxWidth.zero.html canvas/philip/tests/2d.text.draw.fill.rtl.html canvas/philip/tests/2d.text.draw.fill.basic.html canvas/philip/tests/2d.text.draw.baseline.alphabetic.html canvas/philip/tests/2d.text.draw.fontface.repeat.html canvas/philip/tests/2d.text.draw.space.basic.html canvas/philip/tests/2d.text.draw.fontface.notinpage.html canvas/philip/tests/2d.text.draw.align.start.ltr.html canvas/philip/tests/2d.text.draw.fill.maxWidth.large.html canvas/philip/tests/2d.text.draw.stroke.unaffected.html canvas/philip/tests/2d.text.measure.width.basic.html canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html

Created attachment 156795 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 156749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156749&action=review > Source/WebCore/rendering/TextAutosizer.cpp:89 > + Element* element = node->isElementNode() ? toElement(node) : node->parentElement(); I fear that nothing prevents this line to return the same |element| if you have several RenderText parented under the same element. You seem to work-around that by storing a |textSizeMultiplier| that you can set it several times. > Source/WebCore/rendering/TextAutosizer.cpp:93 > - RefPtr<RenderStyle> style = RenderStyle::clone(text->style()); > - FontDescription fontDescription(style->fontDescription()); > - fontDescription.setComputedSize(newSize); > - style->setFontDescription(fontDescription); > - style->font().update(style->font().fontSelector()); > - text->setStyle(style.release()); > + element->textSizeMultiplier = multiplier; The source of most your issues are related to by-passing the style recalculation logic. Could you substantiate why you need to do that apart from the above explanation? (I couldn't find an explanation why the previous code was bad) > Source/WebCore/rendering/TextAutosizer.cpp:102 > bool TextAutosizer::treatAsInline(const RenderObject* renderer) > { > - return !renderer->isRenderBlock() || renderer->isListItem() || renderer->isInlineBlockOrInlineTable(); > + return !renderer->isRenderBlock() || renderer->isAnonymous() || renderer->isListItem() || renderer->isInlineBlockOrInlineTable(); How about flex-boxes? inline-grids? This function is super badly named on top of not being |const|. I don't understand what you are trying to do here: you seem to ignore some renderers but it seems completely arbitrary. Why wouldn't you change tables' cells' text font size? How about inline-blocks? Why don't you walk into anonymous blocks as they could contain inline renderers?

Thanks for taking a look Julien! (In reply to comment #18) > (From update of attachment 156749 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156749&action=review > > > Source/WebCore/rendering/TextAutosizer.cpp:89 > > + Element* element = node->isElementNode() ? toElement(node) : node->parentElement(); > > I fear that nothing prevents this line to return the same |element| if you have several RenderText parented under the same element. You seem to work-around that by storing a |textSizeMultiplier| that you can set it several times. Yes, TextAutosizer::processText gets called for each RenderText being boosted, but they all just set the textSizeMultiplier of their enclosing Element, hence they often overwrite each other (though fortunately the value should generally be the same assuming the enclosing treatAsInline=false RenderBlock is not a child of the enclosing Element). This does seem quite redundant. > > Source/WebCore/rendering/TextAutosizer.cpp:93 > > - RefPtr<RenderStyle> style = RenderStyle::clone(text->style()); > > - FontDescription fontDescription(style->fontDescription()); > > - fontDescription.setComputedSize(newSize); > > - style->setFontDescription(fontDescription); > > - style->font().update(style->font().fontSelector()); > > - text->setStyle(style.release()); > > + element->textSizeMultiplier = multiplier; > > The source of most your issues are related to by-passing the style recalculation logic. Could you substantiate why you need to do that apart from the above explanation? (I couldn't find an explanation why the previous code was bad) What would be the correct way of triggering the style recalculation logic? The main problem with the old code was that it would successfully set the font size of the text node, but next time Element::recalcStyle got called the font size would revert to its specified size (from CSS). A future Text Autosizing pass would often then reapply the multiplier, but a) that seems inefficient b) it seems somewhat risky, as there might be unintended consequences of the font size being temporarily wrong every so often (is FrameView::layout guaranteed to get called soon after every Element::recalcStyle?) c) it's quite hard to do reliably (such that sizes don't unexpectedly change). Downstream in Clank I went so far as maintaining a list of text nodes that are currently being autosized, and their corresponding multipliers, so I can keep them autosized at a stable size even if they later stop requiring autosizing (e.g. if their block becomes narrower). That said it's still possible the old way was still better - what do you think? > > Source/WebCore/rendering/TextAutosizer.cpp:102 > > bool TextAutosizer::treatAsInline(const RenderObject* renderer) > > { > > - return !renderer->isRenderBlock() || renderer->isListItem() || renderer->isInlineBlockOrInlineTable(); > > + return !renderer->isRenderBlock() || renderer->isAnonymous() || renderer->isListItem() || renderer->isInlineBlockOrInlineTable(); > > How about flex-boxes? inline-grids? This function is super badly named on top of not being |const|. > > I don't understand what you are trying to do here: you seem to ignore some renderers but it seems completely arbitrary. Why wouldn't you change tables' cells' text font size? How about inline-blocks? Why don't you walk into anonymous blocks as they could contain inline renderers? I agree that this is badly named. Nothing gets ignored, I'm just drawing an arbitrary boundary. In processSubtree, it walks the tree to find all RenderBlocks which treatAsInline considers "not inline". Then, for each of those blocks, processBlock walks the subtree of that block, excluding any other "not inline" blocks (which will be processed by their own call to processBlock) or their children, to find all the descendants which aren't RenderBlocks or which treatAsInline considers to be "inline". So every renderer gets processed eventually, but they are processed in clumps (currently RenderBlocks for which treatAsInline returns false) over which it is desirable for the text size multiplier to be uniform. The exact definition of these clumps is up for debate. The text size multiplier must be consistent within each given line, which is why inline blocks and tables aren't allowed to form their own clump (for these purposes a single-line inline block/table is essentially just a span with different wrapping behavior). List items should also all have the same font size (unless the webpage explicitly requested otherwise), which is why they are also not allowed to form their own clump, even though they're technically separate blocks. I'm having second thoughts about anonymous blocks, as I can't think of a strong requirement for them being consistent with their parent non-anonymous block, though in practice that is likely to often be preferable. I strongly recommend reading Mozilla's description of how they do Font Inflation: http://jwir3.wordpress.com/2012/07/30/font-inflation-fennec-and-you/ as they explain this better than me. Their equivalent concept to treatAsInline=false is a "font inflation container". Thanks!

Created attachment 157761 [details] Patch Ok, after an in depth discussion with jchaffraix I've got a much better understanding of how things work and the best way to proceed. This patch reverts to the approach taken by the first patch I uploaded on this bug, but does so in a cleaner, simpler way (notably, without adding an unnecessary specifiedLineHeight field to RenderStyle, as instead this is derived automatically from the textAutosizingMultiplier). The added Layout Tests now all pass, and the rendering looks good even on relatively complex pages like wikipedia articles. This is now ready for review.

Comment on attachment 157761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157761&action=review > Source/WebCore/rendering/style/RenderStyle.h:207 > + // for just one float, but putting it in e.g. StyleRareNonInheritedData seems costly > + // as it's not rare, so most RenderText and their parents would need an instance of it. > + // Perhaps I should create a StyleNonInheritedData, and move m_box, visual, > + // m_background and surround to be members of that (like StyleRareNonInheritedData has btw how do we currently decide what is rare and not?

Comment on attachment 157761 [details] Patch Attachment 157761 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13477426 New failing tests: transitions/font-family-during-transition.html fast/text-autosizing/font-scale-factor.html

Created attachment 157854 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 157982 [details] Patch Fixed initialization of RenderStyle::m_textAutosizingMultiplier which was causing transitions/font-family-during-transition.html to fail, and updated the expectations for fast/text-autosizing/font-scale-factor.html now this patch has fixed line-height. Also added a new test, fast/text-autosizing/em-margin-border-padding.html, which documents the behaviour of em margin/border/padding lengths (i.e. they aren't currently scaled).

(In reply to comment #21) > btw how do we currently decide what is rare and not? I'm not sure. The comment in StyleRareNonInheritedData.h just says: // This struct is for rarely used non-inherited CSS3, CSS2, and WebKit-specific properties. // By grouping them together, we save space, and only allocate this object when someone // actually uses one of these properties. But it doesn't define "rarely used"...

Comment on attachment 157982 [details] Patch Attachment 157982 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13489201

Created attachment 157988 [details] Patch Oops, added #if ENABLE(TEXT_AUTOSIZING) around changes to RenderStyle.cpp to fix Mac EWS.

Comment on attachment 157988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157988&action=review This is getting close. I think another round and it will be OK to land. > Source/WebCore/rendering/TextAutosizer.cpp:90 > + renderer->setStyle(newStyle.release()); > + renderer->setNeedsLayoutAndPrefWidthsRecalc(); You shouldn't need to call setNeedsLayoutAndPrefWidthsRecalc as setStyle should do the proper invalidation (due to the check in RenderStyle::diff) > Source/WebCore/rendering/style/RenderStyle.cpp:1198 > + desc.setComputedSize(specifiedSize * textAutosizingMultiplier()); // FIXME: This is overly simplistic. That's not really an actionable FIXME, what really needs to be fixed? > Source/WebCore/rendering/style/RenderStyle.h:210 > + // FIXME: Where should this go? Doesn't seem worth creating a StyleTextAutosizingData > + // for just one float, but putting it in e.g. StyleRareNonInheritedData seems costly > + // as it's not rare, so most RenderText and their parents would need an instance of it. > + // Perhaps I should create a StyleNonInheritedData, and move m_box, visual, > + // m_background and surround to be members of that (like StyleRareNonInheritedData has > + // a bunch of DataRef members)? > + float m_textAutosizingMultiplier; That's a tricky question. You don't want to increase the size of RenderStyle as you do now so that rules out a new DataRef pointer (which would increase RenderStyle more than just adding the member) or this approach. I would add it to StyleVisualData for those reason: it's a non-inherited visual information akin to zoom, the structure is small and it's also supposed to be not-rare. > Source/WebCore/rendering/style/RenderStyle.h:632 > + Length specifiedLineHeight() const { return inherited->line_height; } Exposing this function would remove some #if guard in the code so I would rather expose it and remove the #if-def casing. > Source/WebCore/rendering/style/RenderStyle.h:639 > + if (textAutosizingMultiplier() > 1 && lh.isFixed()) > + return Length(lh.value() * textAutosizingMultiplier(), Fixed); You never actually scale any percent or calc values, is that really fine? By the way, you could wrap only those 2 lines with the #if guard to limit the difference. > Source/WebCore/rendering/style/RenderStyle.h:1155 > + void setFontSize(float specifiedSize); Bug 18091 introduced this function with the intent that it should be used only during animating. By removing the 'blend' part of it, you are basically allowing anyone to override the font-size. I think it's fine to do this renaming but I would like a comment left in its place to underline that it shouldn't be done lightly.

Adding Darin / Dave in case they have a comment about the RenderStyle::setBlendFontSize -> setFontSize renaming. This would basically open other call sites to use it inside the rendering code, which a couple of places already do.

Comment on attachment 157988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157988&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:90 >> + renderer->setNeedsLayoutAndPrefWidthsRecalc(); > > You shouldn't need to call setNeedsLayoutAndPrefWidthsRecalc as setStyle should do the proper invalidation (due to the check in RenderStyle::diff) Done (yes, I was wondering about that). >> Source/WebCore/rendering/style/RenderStyle.cpp:1198 >> + desc.setComputedSize(specifiedSize * textAutosizingMultiplier()); // FIXME: This is overly simplistic. > > That's not really an actionable FIXME, what really needs to be fixed? Changed to "FIXME: Large font sizes needn't be multiplied as much since they are already more legible." >> Source/WebCore/rendering/style/RenderStyle.h:210 >> + float m_textAutosizingMultiplier; > > That's a tricky question. You don't want to increase the size of RenderStyle as you do now so that rules out a new DataRef pointer (which would increase RenderStyle more than just adding the member) or this approach. > > I would add it to StyleVisualData for those reason: it's a non-inherited visual information akin to zoom, the structure is small and it's also supposed to be not-rare. That sounds good - done. >> Source/WebCore/rendering/style/RenderStyle.h:632 >> + Length specifiedLineHeight() const { return inherited->line_height; } > > Exposing this function would remove some #if guard in the code so I would rather expose it and remove the #if-def casing. Done (much neater!) >> Source/WebCore/rendering/style/RenderStyle.h:639 >> + return Length(lh.value() * textAutosizingMultiplier(), Fixed); > > You never actually scale any percent or calc values, is that really fine? > > By the way, you could wrap only those 2 lines with the #if guard to limit the difference. Percent values are proportional to the current font size, so these should already scale. I've added a FIXME for calc values, as they're kinda messy. Done re the #if guard. >> Source/WebCore/rendering/style/RenderStyle.h:1155 >> + void setFontSize(float specifiedSize); > > Bug 18091 introduced this function with the intent that it should be used only during animating. By removing the 'blend' part of it, you are basically allowing anyone to override the font-size. I think it's fine to do this renaming but I would like a comment left in its place to underline that it shouldn't be done lightly. Added comment "// Only used for blending font sizes when animating, or MathML anonymous blocks, or text autosizing."

Created attachment 158273 [details] Patch Addressed jchaffraix's review comments.

Comment on attachment 158273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158273&action=review > Source/WebCore/rendering/TextAutosizer.cpp:61 > + if (!isNotAutosizingContainer(descendant)) Doesn't Non sounds better than Not here? > Source/WebCore/rendering/style/RenderStyle.cpp:1178 > + // FIXME: Large font sizes needn't be multiplied as much since they are already more legible. But then again you need to make sure they stay larger to keep the intended layout

Comment on attachment 158273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158273&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:61 >> + if (!isNotAutosizingContainer(descendant)) > > Doesn't Non sounds better than Not here? Not seems slightly clearer to me. Non suggests that it's the container that can "autosize" or not, whereas this is intended to be short for "is not container for Text Autosizing". I agree that the double negative is ugly though :| >> Source/WebCore/rendering/style/RenderStyle.cpp:1178 >> + // FIXME: Large font sizes needn't be multiplied as much since they are already more legible. > > But then again you need to make sure they stay larger to keep the intended layout Indeed. The algorithm I currently have downstream for this is: // Somewhat arbitrary "pleasant" font size. const float minZoomFontSize = 15; // Boost fonts that the page author has specified to be larger than // minZoomFontSize by less and less, until huge fonts are not boosted at all. // For specifiedSize between 0 and minZoomFontSize we directly apply the // multiplier; hence for specifiedSize == minZoomFontSize, boostedSize (output // font size) will be multiplier * minZoomFontSize. For greater specifiedSizes // we want to gradually fade out the amount of font boosting, so for // every 1px increase in specifiedSize beyond minZoomFontSize we will only // increase boostedSize by RATE_OF_INCREASE_OF_BOOSTED_SIZE_AFTER_MIN_SIZE // until no font boosting is occuring, after which we stop boosting the text // (which is equivalent to setting boostedSize to specifiedSize, i.e. for // every 1px increase in specifiedSize, boostedSize will increase by 1px). const float RATE_OF_INCREASE_OF_BOOSTED_SIZE_AFTER_MIN_SIZE = 0.5f; float boostedSize; if (specifiedSize <= minZoomFontSize) boostedSize = multiplier * specifiedSize; else { boostedSize = multiplier * minZoomFontSize + RATE_OF_INCREASE_OF_BOOSTED_SIZE_AFTER_MIN_SIZE * (specifiedSize - minZoomFontSize); if (boostedSize <= specifiedSize) { boostedSize = specifiedSize; } } boostedSize = roundf(boostedSize); Mozilla used to have a different behavior that flattened font sizes more and didn't seem great, though they've recently changed this (https://bugzilla.mozilla.org/show_bug.cgi?id=777089), and I haven't had time to evaluate whether their new approach is better.

Comment on attachment 158273 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158273&action=review >>> Source/WebCore/rendering/TextAutosizer.cpp:61 >>> + if (!isNotAutosizingContainer(descendant)) >> >> Doesn't Non sounds better than Not here? > > Not seems slightly clearer to me. Non suggests that it's the container that can "autosize" or not, whereas this is intended to be short for "is not container for Text Autosizing". I agree that the double negative is ugly though :| Changed to isNotAnAutosizingContainer at beverloo's suggestion.

> Mozilla used to have a different behavior that flattened font sizes more and didn't seem great, though they've recently changed this (https://bugzilla.mozilla.org/show_bug.cgi?id=777089), and I haven't had time to evaluate whether their new approach is better. Interesting, thanks for sharing. Their calculation seems quite easy to implement as well, so it would be interesting to evaluate when you intent to upstream the other part. I wonder if they have some test cases we could share.

Created attachment 158296 [details] Patch Renamed isNotAutosizingContainer to isNotAnAutosizingContainer.

Comment on attachment 158296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158296&action=review > Source/WebCore/ChangeLog:14 > + renamed isNotAutosizingContainer to clarify that its purpose is to Need to update the changelog as well :-(

Created attachment 158298 [details] Patch Oops, updated ChangeLog.

Comment on attachment 158298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158298&action=review > Source/WebCore/ChangeLog:59 > + (StyleVisualData): Please fill those entries, an explanation at the beginning is not a replacement for those. Especially since you are doing a couple of changes that are not mentioned in the explanation. > Source/WebCore/rendering/TextAutosizer.cpp:61 > + if (!isNotAnAutosizingContainer(descendant)) I find the double negation to be difficult to read. You can easily remove it: if (isNotAnAutosizingContainer(descendant)) continue; processBlock(toRenderBlock(descendant), windowSize); > Source/WebCore/rendering/TextAutosizer.cpp:76 > + for (RenderObject* descendant = traverseNext(block, block, isNotAnAutosizingContainer); descendant; descendant = traverseNext(descendant, block, isNotAnAutosizingContainer)) { traverseNext is pretty confusing but this is orthogonal: the name is bad (super generic that doesn't really say *which* part of the tree you actually traverse) but most the filter parameter doesn't add much. > Source/WebCore/rendering/TextAutosizer.cpp:94 > + return !renderer->isRenderBlock() || renderer->isListItem() || renderer->isInline(); A comment on what type of renderers you actually consider and *why* would be useful. > Source/WebCore/rendering/style/RenderStyle.cpp:1173 > +void RenderStyle::setFontSize(float specifiedSize) I don't understand why you changed the parameter to float here. > Source/WebCore/rendering/style/RenderStyle.cpp:1184 > + if (desc.specifiedSize()) > + desc.setComputedSize(specifiedSize * desc.computedSize() / desc.specifiedSize()); > + else > + desc.setComputedSize(specifiedSize); This code doesn't look right. You are using specifiedSize *before* setting it. This should be the 2 previous lines without any extra massaging or you need to explain why we need that. > Source/WebCore/rendering/style/RenderStyle.h:601 > + // FIXME: This should probably return a float, i.e. fontDescription().computedSize() As mentioned I don't think this FIXME is right so let's not put it from this change. > Source/WebCore/rendering/style/RenderStyle.h:1146 > + // Only used for blending font sizes when animating, or MathML anonymous blocks, or text autosizing. It's more 'and' than 'or': // Only used for blending font sizes when animating, MathML anonymous blocks and text autosizing. > Source/WebCore/rendering/style/StyleVisualData.h:58 > + float textAutosizingMultiplier; Let's follow WebKit style here. This should be m_textAutosizingMultiplier. > LayoutTests/fast/text-autosizing/wide-child.html:1 > +<html> Please add a doctype here. We don't want to test in quirks mode. It should be added to all tests.

Comment on attachment 158298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158298&action=review >> Source/WebCore/ChangeLog:59 >> + (StyleVisualData): > > Please fill those entries, an explanation at the beginning is not a replacement for those. Especially since you are doing a couple of changes that are not mentioned in the explanation. Ah, sure - in fact I see webkit-patch has recently been updated to recommend filing these in :) Is this the expected amount of detail, or am I just waffling? And should I wrap lines before they reach 500 chars, or is that ok? :) >> Source/WebCore/rendering/TextAutosizer.cpp:61 >> + if (!isNotAnAutosizingContainer(descendant)) > > I find the double negation to be difficult to read. You can easily remove it: > > if (isNotAnAutosizingContainer(descendant)) > continue; > > processBlock(toRenderBlock(descendant), windowSize); Done. >> Source/WebCore/rendering/TextAutosizer.cpp:76 >> + for (RenderObject* descendant = traverseNext(block, block, isNotAnAutosizingContainer); descendant; descendant = traverseNext(descendant, block, isNotAnAutosizingContainer)) { > > traverseNext is pretty confusing but this is orthogonal: the name is bad (super generic that doesn't really say *which* part of the tree you actually traverse) but most the filter parameter doesn't add much. The filter parameter is crucial: unlike having an if(!filter)continue; in the loop body, if the filter fails, that entire subtree of the descendants is skipped (even if there are matching objects deeper within it). I've moved this method to RenderObject::nextInPreOrderMatchingFilter, where it complements the existing nextInPreOrder* methods. Hopefully this is cleaner now. >> Source/WebCore/rendering/TextAutosizer.cpp:94 >> + return !renderer->isRenderBlock() || renderer->isListItem() || renderer->isInline(); > > A comment on what type of renderers you actually consider and *why* would be useful. Done. >> Source/WebCore/rendering/style/RenderStyle.cpp:1173 >> +void RenderStyle::setFontSize(float specifiedSize) > > I don't understand why you changed the parameter to float here. specifiedSize is stored internally (in FontDescription) as a float, so accepting a float makes sense, and it allows me to call setFontSize(fontDescription().specifiedSize()) in RenderStyle::setTextAutosizingMultiplier without the specifiedSize getting truncated to an int. This admittedly has the side effect that users of setBlendedFontSize which were passing in floats and expecting them to get truncated won't have them truncated any more. RenderMathMLSubSup.cpp was already casting to int. I've added a cast to int in RenderThemeBlackBerry.cpp. That leaves CSSPropertyAnimation, where I can't easily add an int cast, since it uses function pointers. In my testing (with slowed down transitions) I can't see any visual difference caused by directly using the floating-point blended specifiedSize (i.e. something further down the font rendering path seems to be truncating to int anyway); however when text with a font-size transition is autosized, it actually looks much better without truncating to int. For example if the specifiedSize gets transitioned from 10px to 20px over 2 seconds there will be just (20-10) = 10 font size changes (5 FPS) if truncating the specifiedSize to int. But supposing the textAutosizingMultiplier is 3x, then the computedSize will be actually transitioning from 30px to 60px, and if you truncate the specifiedSize that will still happen in the same 10 steps (except each is now 3px of computedSize change), whereas if you don't truncate the specifiedSize to int you will get the full (60-30) = 30 font size changes (15 FPS) which looks much better (the details of this will change slightly once we stop directly multiplying font sizes, but the overall effect will remain the same). >> Source/WebCore/rendering/style/RenderStyle.cpp:1184 >> + desc.setComputedSize(specifiedSize); > > This code doesn't look right. You are using specifiedSize *before* setting it. This should be the 2 previous lines without any extra massaging or you need to explain why we need that. I added a comment "// Preserve existing ratio between specifiedSize and computedSize.". The aim is to set newComputedSize = newSpecifiedSize * (oldComputedSize/oldSpecifiedSize), since most of the setters don't know or care about computedSize, and it usually makes sense to preserve the existing ratio (which will typically either be 1 or the old-style text zoom). For this I have to use the value of specifiedSize before I overwrite it with the new value. It's debatable whether this is really necessary, but doing everything in specifiedSize certainly makes it easier for setBlendedFontSize to be compatible with both text zoom and Text Autosizing. I notice though that I've fallen into the trap of assuming that ENABLE(TEXT_AUTOSIZING) means Text Autosizing is enabled, when actually it might be disabled at runtime. I've changed this so it also preserves the ratio if Text Autosizing is runtime disabled (or if there isn't a textAutosizingMultiplier for any other reason, but in practice the computed/specified ratio should always be 1 if Text Autosizing is enabled but the renderer isn't being autosized. Does this sound reasonable now? >> Source/WebCore/rendering/style/RenderStyle.h:601 >> + // FIXME: This should probably return a float, i.e. fontDescription().computedSize() > > As mentioned I don't think this FIXME is right so let's not put it from this change. Done. >> Source/WebCore/rendering/style/RenderStyle.h:1146 >> + // Only used for blending font sizes when animating, or MathML anonymous blocks, or text autosizing. > > It's more 'and' than 'or': > // Only used for blending font sizes when animating, MathML anonymous blocks and text autosizing. Done. >> Source/WebCore/rendering/style/StyleVisualData.h:58 >> + float textAutosizingMultiplier; > > Let's follow WebKit style here. This should be m_textAutosizingMultiplier. My understanding is that in the rare cases where public data members are used, they should be unprefixed: https://lists.webkit.org/pipermail/webkit-dev/2010-June/013037.html >> LayoutTests/fast/text-autosizing/wide-child.html:1 >> +<html> > > Please add a doctype here. We don't want to test in quirks mode. It should be added to all tests. Done.

Created attachment 158615 [details] Patch Addressed jchaffraix's 2nd round review comments.

Comment on attachment 158615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158615&action=review > Source/WebCore/ChangeLog:31 > + * css/StyleBuilder.cpp: > + (WebCore::ApplyPropertyLineHeight::applyValue): Replaced Length(-100.0, Percent) with RenderStyle::initialLineHeight(), which is equivalent but makes the intent clearer. > + (WebCore::ApplyPropertyLineHeight::createHandler): Use specifiedLineHeight to match setLineHeight which takes a specified line height (and generally this avoids accidentally inheriting text autosizing). > + * css/StyleResolver.cpp: > + (WebCore::StyleResolver::applyProperty): Similarly use specifiedLineHeight when inheriting line height. > + * page/animation/CSSPropertyAnimation.cpp: I would really wrap those. Sometimes I think adding a few newlines and identation, it also becomes a lot more readable. I think readability here is very important. What I have done myself, is to add a newline for long lines, then indent and wrap and then add another newline.

Comment on attachment 158615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158615&action=review > Source/WebCore/ChangeLog:43 > + (WebCore::TextAutosizer::processBox): Renamed from processBlock, since RenderBoxes are sufficient; use renamed nextInPreOrderMatchingFilter; make two calls to setMultiplier (one for parent for line height) instead of processText. I would make this: (WebCore::TextAutosizer::processBox): Renamed from processBlock, since RenderBoxes are sufficient; use renamed nextInPreOrderMatchingFilter; make two calls to setMultiplier (one for parent for line height) instead of processText. (WebCore::TextAutosizer::setMultiplier): Maybe that is over doing it, but at least it is easy to read

Created attachment 158626 [details] Patch Wrapped ChangeLog as suggested by kenneth. Let me know if this is excessively verbose!

I find the changelog pretty good now.

Created attachment 158830 [details] Patch Rebased, following conflict with http://webkit.org/b/94019.

Comment on attachment 158830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158830&action=review > I find the changelog pretty good now. I totally agree. > Source/WebCore/rendering/RenderObject.cpp:371 > +RenderObject* RenderObject::nextInPreOrderMatchingFilter(const RenderObject* stayWithin, RenderObjectFilterFunctor filter) const The name is a bit of a misnomer: I would expect this function to be a full preorder traversal, which it's not as it skips subtrees that don't match your filter. I thought of nextInPreOrderIgnoringNonMatchingSubtrees for a replacement but it's long and again doesn't convey the idea well so we may as well stick with your naming as it's better. It's a pretty narrow use case which I am not convinced is worth exposing in RenderObject. Let's keep it in TextAutosizer for now as this will force a world rebuild for little value. > Source/WebCore/rendering/style/RenderStyle.cpp:-1190 > -int RenderStyle::wordSpacing() const { return inherited->font.wordSpacing(); } > -int RenderStyle::letterSpacing() const { return inherited->font.letterSpacing(); } Apparently unintended change (not explained anywhere). > Source/WebCore/rendering/style/RenderStyle.cpp:1215 > + // Fall through, since Text Autosizing might not be runtime enabled. I would rather have explicit curly braces around the 'else' body below to avoid relying on the implicit rule that the 'else' should be 1 line long. > Source/WebCore/rendering/style/RenderStyle.cpp:1221 > + // Preserve existing ratio between specifiedSize and computedSize. > + if (desc.specifiedSize()) > + desc.setComputedSize(specifiedSize * desc.computedSize() / desc.specifiedSize()); > + else > + desc.setComputedSize(specifiedSize); > It's debatable whether this is really necessary, but doing everything in specifiedSize certainly makes it easier for setBlendedFontSize to be compatible with both text zoom and Text Autosizing. If it's not strictly necessary, I would prefer for it to be removed from this patch. This code is used by other code paths which makes the risk of regressions real but also your path is big enough without this change. > Source/WebCore/rendering/style/StyleVisualData.h:58 > + float textAutosizingMultiplier; > My understanding is that in the rare cases where public data members are used, they should be unprefixed: > https://lists.webkit.org/pipermail/webkit-dev/2010-June/013037.html That's not my reading. Here you are in a class, not a struct so the discussion doesn't apply. Looking at the code, there is a mix between the 2 styles but the new code mostly uses m_ prefixing even if the variable is public so we should use that for consistency. > LayoutTests/fast/text-autosizing/em-margin-border-padding.html:15 > + console.warn("This test depends on the Text Autosizing setting being true, so run it in DumpRenderTree, or manually enable Text Autosizing, and either use a mobile device with 320px device-width (like Nexus S or iPhone), or define HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP."); HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP doesn't work in WebKit AFAICT so it would be better if we didn't mention that (especially since there is HACK in it)

Comment on attachment 158830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158830&action=review Cool, I'll make those changes tomorrow. >> Source/WebCore/rendering/style/RenderStyle.cpp:-1190 >> -int RenderStyle::letterSpacing() const { return inherited->font.letterSpacing(); } > > Apparently unintended change (not explained anywhere). I'm just moving these from where http://webkit.org/b/94019 left them, arbitrarily sitting between fontSize and lineHeight (which it is useful to have close together since the symmetry is important) to instead be next to their setters below the line height methods. >> LayoutTests/fast/text-autosizing/em-margin-border-padding.html:15 >> + console.warn("This test depends on the Text Autosizing setting being true, so run it in DumpRenderTree, or manually enable Text Autosizing, and either use a mobile device with 320px device-width (like Nexus S or iPhone), or define HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP."); > > HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP doesn't work in WebKit AFAICT so it would be better if we didn't mention that (especially since there is HACK in it) It's just an ifdef in Settings.cpp. Surely most ways of building WebKit have some way to set arbitrary compile flags?

Comment on attachment 158830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158830&action=review >>> Source/WebCore/rendering/style/RenderStyle.cpp:-1190 >>> -int RenderStyle::letterSpacing() const { return inherited->font.letterSpacing(); } >> >> Apparently unintended change (not explained anywhere). > > I'm just moving these from where http://webkit.org/b/94019 left them, arbitrarily sitting between fontSize and lineHeight (which it is useful to have close together since the symmetry is important) to instead be next to their setters below the line height methods. This is another orthogonal change that should be pushed to a follow-up patch. As implied earlier, I consider your patch to be at the limit of what can be reviewed so anything that don't need to be there, shoudn't. >>> LayoutTests/fast/text-autosizing/em-margin-border-padding.html:15 >>> + console.warn("This test depends on the Text Autosizing setting being true, so run it in DumpRenderTree, or manually enable Text Autosizing, and either use a mobile device with 320px device-width (like Nexus S or iPhone), or define HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP."); >> >> HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP doesn't work in WebKit AFAICT so it would be better if we didn't mention that (especially since there is HACK in it) > > It's just an ifdef in Settings.cpp. Surely most ways of building WebKit have some way to set arbitrary compile flags? I missed the compile flag. It's unfortunate that we need to rely on some compile-time hack to test this feature on desktop.

Comment on attachment 158830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158830&action=review >> Source/WebCore/rendering/RenderObject.cpp:371 >> +RenderObject* RenderObject::nextInPreOrderMatchingFilter(const RenderObject* stayWithin, RenderObjectFilterFunctor filter) const > > The name is a bit of a misnomer: I would expect this function to be a full preorder traversal, which it's not as it skips subtrees that don't match your filter. > > I thought of nextInPreOrderIgnoringNonMatchingSubtrees for a replacement but it's long and again doesn't convey the idea well so we may as well stick with your naming as it's better. > > It's a pretty narrow use case which I am not convinced is worth exposing in RenderObject. Let's keep it in TextAutosizer for now as this will force a world rebuild for little value. Done (and added a comment, "// Use to traverse the tree of descendants, excluding any subtrees within that whose root doesn't pass the filter.", to try and explain its purpose). >>>> Source/WebCore/rendering/style/RenderStyle.cpp:-1190 >>>> -int RenderStyle::letterSpacing() const { return inherited->font.letterSpacing(); } >>> >>> Apparently unintended change (not explained anywhere). >> >> I'm just moving these from where http://webkit.org/b/94019 left them, arbitrarily sitting between fontSize and lineHeight (which it is useful to have close together since the symmetry is important) to instead be next to their setters below the line height methods. > > This is another orthogonal change that should be pushed to a follow-up patch. As implied earlier, I consider your patch to be at the limit of what can be reviewed so anything that don't need to be there, shoudn't. Ok, I've moved all of these to their original positions. >> Source/WebCore/rendering/style/RenderStyle.cpp:1215 >> + // Fall through, since Text Autosizing might not be runtime enabled. > > I would rather have explicit curly braces around the 'else' body below to avoid relying on the implicit rule that the 'else' should be 1 line long. In the end I just made us set the computedSize a second time (which'll hopefully be optimized out) if the conditions were satisfied, overwriting the original value. This seems cleaner? >> Source/WebCore/rendering/style/RenderStyle.cpp:1221 >> + desc.setComputedSize(specifiedSize); > > > If it's not strictly necessary, I would prefer for it to be removed from this patch. This code is used by other code paths which makes the risk of regressions real but also your path is big enough without this change. Hmm, I've removed it, but there were ugly consequences. When Text Autosizing is enabled, setFontSize must always be called with a specifiedSize, and will use that to calculate the computedSize. But now that we no longer preserve the ratio, if text zoom is active, setFontSize must always be called with a computedSize (or you'd lose the text zoom). This means I've had to rename the parameter from specifiedSize to size since it can be either type depending on what's active, and I've had to add an ifdef in CSSPropertyAnimation.cpp so that font-size transitions use the specifiedFontSize getter if ENABLE(TEXT_AUTOSIZING), otherwise they use the [computed]fontSize getter. And this is only possible based on the fragile assumption that text zoom can never be enabled when Text Autosizing is compiled in; so far that's true (as AFAIK Chromium never uses text zoom), but it may not last. All in all it seemed quite a bit cleaner for setFontSize to always take a specifiedFontSize as its parameter, and do the appropriate calculation to update computedSize. >> Source/WebCore/rendering/style/StyleVisualData.h:58 >> + float textAutosizingMultiplier; > > > That's not my reading. Here you are in a class, not a struct so the discussion doesn't apply. Looking at the code, there is a mix between the 2 styles but the new code mostly uses m_ prefixing even if the variable is public so we should use that for consistency. Done. >>>> LayoutTests/fast/text-autosizing/em-margin-border-padding.html:15 >>>> + console.warn("This test depends on the Text Autosizing setting being true, so run it in DumpRenderTree, or manually enable Text Autosizing, and either use a mobile device with 320px device-width (like Nexus S or iPhone), or define HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP."); >>> >>> HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP doesn't work in WebKit AFAICT so it would be better if we didn't mention that (especially since there is HACK in it) >> >> It's just an ifdef in Settings.cpp. Surely most ways of building WebKit have some way to set arbitrary compile flags? > > I missed the compile flag. It's unfortunate that we need to rely on some compile-time hack to test this feature on desktop. I could add a flag to about:flags for testing it in Chromium at least. Would that be useful, or can you think of a more general way of making it available?

Created attachment 159123 [details] Patch Addressed jchaffraix's 3rd round review comments.

Comment on attachment 159123 [details] Patch Attachment 159123 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13512944

Comment on attachment 159123 [details] Patch Attachment 159123 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13530052

Comment on attachment 159123 [details] Patch Attachment 159123 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13514852

Created attachment 159135 [details] Patch > Hmm, I've removed it, but there were ugly consequences.\n\nTurns out as well as that I also needed to expose a float getter for computedFontSize on RenderStyle, since both the getter and setter of a PropertyWrapper must have the same type. This fixes the Mac/Win/gtk builds.

Comment on attachment 159135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159135&action=review r=me. Let's see if it builds on all platforms before cq+ it. > LayoutTests/fast/text-autosizing/em-margin-border-padding.html:15 > + console.warn("This test depends on the Text Autosizing setting being true, so run it in DumpRenderTree, or manually enable Text Autosizing, and either use a mobile device with 320px device-width (like Nexus S or iPhone), or define HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP."); > I could add a flag to about:flags for testing it in Chromium at least. Would that be useful, or can you think of a more general way of making it available? Mhh, I don't have a good answer. A runtime flag would enable us to expose it for sure but it has its downsides. Let's go with that comment and file a follow-up bug as it is an open problem.

(In reply to comment #56) > > I could add a flag to about:flags for testing it in Chromium at least. Would that be useful, or can you think of a more general way of making it available? > > Mhh, I don't have a good answer. A runtime flag would enable us to expose it for sure but it has its downsides. Let's go with that comment and file a follow-up bug as it is an open problem. Filed http://webkit.org/b/94371

Comment on attachment 159135 [details] Patch Clearing flags on attachment: 159135 Committed r125925: <http://trac.webkit.org/changeset/125925>

All reviewed patches have been landed. Closing bug.