Text Autosizing: Cluster text at flow roots, for consistency and to avoid autosizing headers/footers.
Created attachment 164586 [details] Patch This patch should be almost there, one or two comments need tweaking (e.g. in isCluster), but the code is all ready for review.
Comment on attachment 164586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164586&action=review > Source/WebCore/ChangeLog:8 > + This CL has 3 main changes: CL = ? > Source/WebCore/ChangeLog:17 > + 2. Clusters must contain a minimum amount of text in order to be > + autosized (4 lines of text, assuming each char is 1em wide, so about How is this on iOS? > Source/WebCore/ChangeLog:23 > + visually distracting. The rationale is that if a cluster contains > + very few lines of text then it's ok to have to zoom in and pan from > + side to side to read each line, since if there are very few lines of > + text you'll only need to pan across once or twice. Good reasoning > Source/WebCore/ChangeLog:78 > + (WebCore::TextAutosizer::clusterContainsEnoughText): contains enough text sounds a bit vague. Enough for what? clusterShouldBeAutosized? > Source/WebCore/ChangeLog:93 > + isContainer (I no longer need an isCluster filter, since I we no longer ? > Source/WebCore/rendering/TextAutosizer.cpp:71 > + // The layoutRoot could be neither a container nor a cluster, so walk up the tree till we find these. find each of these? > Source/WebCore/rendering/TextAutosizer.cpp:91 > + RenderBlock* lca = cluster; Not better writing lca out? > Source/WebCore/rendering/TextAutosizer.cpp:92 > + // FIXME: Is this the best width to use (does double-tap zoom in to content or content+padding)? I guess to content+padding . You always want a bit of padding around the text > Source/WebCore/rendering/TextAutosizer.cpp:193 > +bool TextAutosizer::isCluster(const RenderObject* renderer) isFormattingCluster? > Source/WebCore/rendering/TextAutosizer.cpp:202 > + if (!isContainer(renderer)) > + return false; > + return !renderer->containingBlock() I would add a new line after first return > LayoutTests/fast/text-autosizing/constrained-and-overflow-auto-ancestor.html:3 > <head> what about adding doctype?
Comment on attachment 164586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164586&action=review I haven't looked at the tests but we definitely want another round for the code side of it. > Source/WebCore/rendering/TextAutosizer.cpp:76 > + if (!container) // This should be rare. > + return false; Are you sure it's not impossible? RenderView should always pass the isContainer check and you always have a containing RenderView. > Source/WebCore/rendering/TextAutosizer.cpp:82 > + if (!cluster) // This should be rare. > + return false; Ditto. > Source/WebCore/rendering/TextAutosizer.cpp:90 > + // FIXME: Find lowest common ancestor of first and last descendant text node (or return). I don't understand this comment or what should be done here instead. > Source/WebCore/rendering/TextAutosizer.cpp:184 > -bool TextAutosizer::isNotAnAutosizingContainer(const RenderObject* renderer) > +bool TextAutosizer::isContainer(const RenderObject* renderer) Please let's keep the Autosizing (actually AutoSizing) part in the name. isContainer() is ambiguous. > Source/WebCore/rendering/TextAutosizer.cpp:197 > + // FIXME: Explain basis in flow roots. How about now? Probably no one knows what 'basis' you are talking about. > Source/WebCore/rendering/TextAutosizer.cpp:201 > + if (!isContainer(renderer)) > + return false; Most of the code path have done this check before, it may be worth either ASSERT'ing or not doing them on the caller's side? >> Source/WebCore/rendering/TextAutosizer.cpp:202 >> + return !renderer->containingBlock() > > I would add a new line after first return Shouldn't !renderer->containingBlock() equates renderer->isRenderView() in your case as the tree is assumed to be rooted? > Source/WebCore/rendering/TextAutosizer.cpp:207 > + || (renderer->containingBlock() && renderer->containingBlock()->isHorizontalWritingMode() != renderer->isHorizontalWritingMode()); The renderer->containingBlock() check is unneeded as you do it above anyway. > Source/WebCore/rendering/TextAutosizer.cpp:219 > + // Don't autosize clusters that contain less than 4 lines of text (in > + // practice less lines are required, since measureDescendantTextWidth assumes > + // that characters are 1em wide, but most characters are narrower than that). This comment is not consistent if most characters are *narrower*, you would need *more* than 4 lines of text, not less. > Source/WebCore/rendering/TextAutosizer.cpp:229 > +void TextAutosizer::measureDescendantTextWidth(const RenderBlock* container, float minTextWidth, float* textWidth) Let's use a reference, not a pointer as you don't want the parameter to be optional (you never NULL-checked it).
Comment on attachment 164586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164586&action=review >> Source/WebCore/ChangeLog:8 >> + This CL has 3 main changes: > > CL = ? Changed to "patch" :) >> Source/WebCore/ChangeLog:17 >> + autosized (4 lines of text, assuming each char is 1em wide, so about > > How is this on iOS? iOS doesn't currently do this kind of clustering. Instead they group together text nodes that have the same values for a selection of text-related styles, then each group uses the average of the text size multipliers required by those text nodes. However that's not a very sound approach, because you can have a <em> in the middle of a narrow paragraph, and the em gets highly multiplied because it gets grouped together with some wide italic text nodes, while the surrounding paragraph remains little multiplied (and vice versa, amongst other problems). This can lead to some really bad inconsistencies, even within the same line of text. iOS partially hides these inconsistencies by capping text size multipliers at 1.7x, which works on some sites but really isn't enough for very wide blocks. Ultimately the iOS clustering seems to cause about as many problems as it solves :| >> Source/WebCore/ChangeLog:78 >> + (WebCore::TextAutosizer::clusterContainsEnoughText): > > contains enough text sounds a bit vague. Enough for what? > > clusterShouldBeAutosized? Done. >> Source/WebCore/ChangeLog:93 >> + isContainer (I no longer need an isCluster filter, since I > > we no longer ? Done. >> Source/WebCore/rendering/TextAutosizer.cpp:71 >> + // The layoutRoot could be neither a container nor a cluster, so walk up the tree till we find these. > > find each of these? Done. >> Source/WebCore/rendering/TextAutosizer.cpp:76 >> + return false; > > Are you sure it's not impossible? RenderView should always pass the isContainer check and you always have a containing RenderView. Removed (I wasn't sure if render trees were always topped by a RenderView - is that definitely always the case?). >> Source/WebCore/rendering/TextAutosizer.cpp:82 >> + return false; > > Ditto. Done. >> Source/WebCore/rendering/TextAutosizer.cpp:90 >> + // FIXME: Find lowest common ancestor of first and last descendant text node (or return). > > I don't understand this comment or what should be done here instead. Expanded on this: // FIXME: Many pages set a max-width on their content. So especially for the RenderView, // instead of just taking the width of |cluster| we should find the lowest common ancestor of // the first and last descendant text node of the cluster (i.e. the deepest wrapper block that // contains all the text), and use its width instead. Is that clearer? >> Source/WebCore/rendering/TextAutosizer.cpp:91 >> + RenderBlock* lca = cluster; > > Not better writing lca out? Done. I kept lcaWidth for now to avoid things getting too long - does that seem reasonable? >> Source/WebCore/rendering/TextAutosizer.cpp:92 >> + // FIXME: Is this the best width to use (does double-tap zoom in to content or content+padding)? > > I guess to content+padding . You always want a bit of padding around the text Yeah, but some sites use padding for layout purposes, and I wouldn't want Text Autosizing to give some blocks huge font-size just because they used padding instead of margin. Similarly for double-tap I'd recommend using content+hardcodedPadding. So I think contentLogicalWidth is the safest option here for avoiding strange edge-cases; I've removed the FIXME. >> Source/WebCore/rendering/TextAutosizer.cpp:184 >> +bool TextAutosizer::isContainer(const RenderObject* renderer) > > Please let's keep the Autosizing (actually AutoSizing) part in the name. isContainer() is ambiguous. Done, though I kept a lowercase S for now for consistency with class TextAutosizer (and I've generally been calling this Text Autosizing, not Text Auto Sizing). If we want to change this we should do it as a followup patch. >> Source/WebCore/rendering/TextAutosizer.cpp:193 >> +bool TextAutosizer::isCluster(const RenderObject* renderer) > > isFormattingCluster? Went with isAutosizingCluster for parity with isAutosizingContainer. >> Source/WebCore/rendering/TextAutosizer.cpp:197 >> + // FIXME: Explain basis in flow roots. > > How about now? Probably no one knows what 'basis' you are talking about. I rewrote this comment. It's now longer, but I'm not sure I made it much clearer ^_^ I also added some new ways to be a cluster (overflow != visible, isFlexibleBoxIncludingDeprecated, or hasColumns) since those all cause flow roots, and all deserve to form new clusters. >> Source/WebCore/rendering/TextAutosizer.cpp:201 >> + return false; > > Most of the code path have done this check before, it may be worth either ASSERT'ing or not doing them on the caller's side? Done. Changed param to a RenderBlock to clarify the implicit contract. >>> Source/WebCore/rendering/TextAutosizer.cpp:202 >>> + return !renderer->containingBlock() >> >> I would add a new line after first return > > Shouldn't !renderer->containingBlock() equates renderer->isRenderView() in your case as the tree is assumed to be rooted? > I would add a new line after first return I grepped through WebCore with: cd Source/WebCore/ && git grep -E -B2 '^ *\|\|' -- '*.h' '*.cpp' | grep -A2 'return' And only found the following two styles: return a || b || c return a || b || c of which the first is easier to read, and seems marginally more common... > !renderer->containingBlock() equates renderer->isRenderView() Yes. I'd put !containingBlock because it seemed a clearer guarantee that we would eventually terminate on a cluster, but thinking about it isRenderView is computationally cheaper, so switched to that. >> Source/WebCore/rendering/TextAutosizer.cpp:207 >> + || (renderer->containingBlock() && renderer->containingBlock()->isHorizontalWritingMode() != renderer->isHorizontalWritingMode()); > > The renderer->containingBlock() check is unneeded as you do it above anyway. I've removed this for now, but I notice a scary comment in RenderObject::containingBlock "return 0; // This can still happen in case of an orphaned tree". Can I guarantee that FrameView::layout hasn't given me an orphaned tree as my layout root, so RenderView is the only case that I would get null containingBlock? >> Source/WebCore/rendering/TextAutosizer.cpp:219 >> + // that characters are 1em wide, but most characters are narrower than that). > > This comment is not consistent if most characters are *narrower*, you would need *more* than 4 lines of text, not less. Appended ", so we're overestimating their contribution to the linecount". Does that make things clearer? The maths is correct. >> Source/WebCore/rendering/TextAutosizer.cpp:229 >> +void TextAutosizer::measureDescendantTextWidth(const RenderBlock* container, float minTextWidth, float* textWidth) > > Let's use a reference, not a pointer as you don't want the parameter to be optional (you never NULL-checked it). Done (using a pointer seemed to make it clearer that it was an outparam, as the caller has to pass &var; and WebCore code doesn't null-check most pointer parameters, but I don't know the what the usual style is for outparams...) >> LayoutTests/fast/text-autosizing/constrained-and-overflow-auto-ancestor.html:3 >> <head> > > what about adding doctype? These tests all already have: <!DOCTYPE html>
Created attachment 164846 [details] Patch
Comment on attachment 164846 [details] Patch Addressed kenneth & jchaffraix's review comments.
Comment on attachment 164586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164586&action=review >>> Source/WebCore/rendering/TextAutosizer.cpp:90 >>> + // FIXME: Find lowest common ancestor of first and last descendant text node (or return). >> >> I don't understand this comment or what should be done here instead. > > Expanded on this: > > // FIXME: Many pages set a max-width on their content. So especially for the RenderView, > // instead of just taking the width of |cluster| we should find the lowest common ancestor of > // the first and last descendant text node of the cluster (i.e. the deepest wrapper block that > // contains all the text), and use its width instead. > > Is that clearer? Definitely, now we can say what are the condition to remove the FIXME. >>> Source/WebCore/rendering/TextAutosizer.cpp:91 >>> + RenderBlock* lca = cluster; >> >> Not better writing lca out? > > Done. I kept lcaWidth for now to avoid things getting too long - does that seem reasonable? You can take an hybrid name like commonAncestor. lca is not a good naming due to the fact that you have to know what it means. >>> Source/WebCore/rendering/TextAutosizer.cpp:184 >>> +bool TextAutosizer::isContainer(const RenderObject* renderer) >> >> Please let's keep the Autosizing (actually AutoSizing) part in the name. isContainer() is ambiguous. > > Done, though I kept a lowercase S for now for consistency with class TextAutosizer (and I've generally been calling this Text Autosizing, not Text Auto Sizing). If we want to change this we should do it as a followup patch. OK, I don't strongly feel about the capital 'S' so consistency is good. >>>> Source/WebCore/rendering/TextAutosizer.cpp:202 >>>> + return !renderer->containingBlock() >>> >>> I would add a new line after first return >> >> Shouldn't !renderer->containingBlock() equates renderer->isRenderView() in your case as the tree is assumed to be rooted? > > [snipped the explanation] I think Kenneth was thinking of adding a new line between the if (!isContainer(renderer)) return false; and this line for clarity. >>> Source/WebCore/rendering/TextAutosizer.cpp:207 >>> + || (renderer->containingBlock() && renderer->containingBlock()->isHorizontalWritingMode() != renderer->isHorizontalWritingMode()); >> >> The renderer->containingBlock() check is unneeded as you do it above anyway. > > I've removed this for now, but I notice a scary comment in RenderObject::containingBlock "return 0; // This can still happen in case of an orphaned tree". Can I guarantee that FrameView::layout hasn't given me an orphaned tree as my layout root, so RenderView is the only case that I would get null containingBlock? Layout is called on stable non-orphaned tree. As such unless the layout root gets confused (which usually means that something bad is going on), it should never happen. > RenderView is the only case that I would get null containingBlock? Yes, RenderView is the root of the render tree. As such it acts as the initial containing block in CSS. >>> Source/WebCore/rendering/TextAutosizer.cpp:219 >>> + // that characters are 1em wide, but most characters are narrower than that). >> >> This comment is not consistent if most characters are *narrower*, you would need *more* than 4 lines of text, not less. > > Appended ", so we're overestimating their contribution to the linecount". Does that make things clearer? The maths is correct. OK, I was confused by the phrasing. Can't think of a better way of putting it. >>> Source/WebCore/rendering/TextAutosizer.cpp:229 >>> +void TextAutosizer::measureDescendantTextWidth(const RenderBlock* container, float minTextWidth, float* textWidth) >> >> Let's use a reference, not a pointer as you don't want the parameter to be optional (you never NULL-checked it). > > Done (using a pointer seemed to make it clearer that it was an outparam, as the caller has to pass &var; and WebCore code doesn't null-check most pointer parameters, but I don't know the what the usual style is for outparams...) It's in WebKit style: "An out argument of a function should be passed by reference except rare cases where it is optional in which case it should be passed by pointer." I think you also fall under this rule: "Precede getters that return values through out arguments with the word "get"." So a better naming would be: getDescendantTextWidth.
Comment on attachment 164586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164586&action=review >>>> Source/WebCore/rendering/TextAutosizer.cpp:91 >>>> + RenderBlock* lca = cluster; >>> >>> Not better writing lca out? >> >> Done. I kept lcaWidth for now to avoid things getting too long - does that seem reasonable? > > You can take an hybrid name like commonAncestor. lca is not a good naming due to the fact that you have to know what it means. Changed lcaWidth to commonAncestorWidth >>>>> Source/WebCore/rendering/TextAutosizer.cpp:202 >>>>> + return !renderer->containingBlock() >>>> >>>> I would add a new line after first return >>> >>> Shouldn't !renderer->containingBlock() equates renderer->isRenderView() in your case as the tree is assumed to be rooted? >> >> > > [snipped the explanation] > > I think Kenneth was thinking of adding a new line between the if (!isContainer(renderer)) return false; and this line for clarity. Ah - added. >>>> Source/WebCore/rendering/TextAutosizer.cpp:229 >>>> +void TextAutosizer::measureDescendantTextWidth(const RenderBlock* container, float minTextWidth, float* textWidth) >>> >>> Let's use a reference, not a pointer as you don't want the parameter to be optional (you never NULL-checked it). >> >> Done (using a pointer seemed to make it clearer that it was an outparam, as the caller has to pass &var; and WebCore code doesn't null-check most pointer parameters, but I don't know the what the usual style is for outparams...) > > It's in WebKit style: > > "An out argument of a function should be passed by reference except rare cases where it is optional in which case it should be passed by pointer." > > I think you also fall under this rule: > > "Precede getters that return values through out arguments with the word "get"." > > So a better naming would be: getDescendantTextWidth. Cool, will stick with reference. Discussed offline and we agreed that measure was a clear enough action verb.
Created attachment 164951 [details] Patch Addressed jchaffraix's last few comments.
Comment on attachment 164951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164951&action=review The new patch is starting to look good. More comments. > Source/WebCore/rendering/TextAutosizer.cpp:85 > +{ ASSERT(isAutosizingCluster(cluster)); > Source/WebCore/rendering/TextAutosizer.cpp:93 > + // FIXME: Make commonAncestorWidth for the RenderView independent of whether the window has a vertical > + // scrollbar, to avoid having to change the multiplier if the document height changes. > + float commonAncestorWidth = lowestCommonAncestor->contentLogicalWidth(); That should be easy to do by adding back the right scrollbar width. You can add an helper on RenderBox to do that. > Source/WebCore/rendering/TextAutosizer.cpp:98 > + int logicalWindowWidth = cluster->isHorizontalWritingMode() ? m_windowSize.width() : m_windowSize.height(); > + int logicalLayoutWidth = cluster->isHorizontalWritingMode() ? m_minLayoutSize.width() : m_minLayoutSize.height(); Nit: This could be an helper function on IntSize akin to FractionalLayoutBoxExtent > Source/WebCore/rendering/TextAutosizer.cpp:126 > { ASSERT(isAutosizingContainer(container)); > Source/WebCore/rendering/TextAutosizer.cpp:136 > // FIXME: Increase list marker size proportionately. Does this comment still applies (cf comment in isAutosizingContainer)? > Source/WebCore/rendering/TextAutosizer.cpp:142 > + processCluster(descendantBlock, descendantBlock, descendant); > + else > + processContainer(multiplier, descendantBlock, descendant); Let's use descendantBlock everywhere here instead of mixing descendant and descendantBlock which makes it look like we are passing different pointers. > Source/WebCore/rendering/TextAutosizer.cpp:230 > +{ ASSERT(isAutosizingCluster(lowestCommonAncestor); > Source/WebCore/rendering/TextAutosizer.cpp:234 > + // than that, so we're overestimating their contribution to the linecount). Would be good to explain 'why' you are not auto-sizing anything less than 4 lines here instead of in the ChangeLog. > Source/WebCore/rendering/TextAutosizer.h:77 > + > + // These two sizes are calculated by processSubtree and cached here. > + IntSize m_windowSize; > + IntSize m_minLayoutSize; Those 2 would be better in a structure that we pass around. As you don't need them after a |processSubtree| phase, it would ensure that no one tries to query them outside it. I wonder if we could throw in the other arguments too (like the |container| and the |multiplier|), feel free to push back on this idea. > LayoutTests/fast/text-autosizing/clusters-sufficient-width-expected.html:32 > +<div style="-webkit-writing-mode: vertical-rl; height: 440px"> > + This text should be autosized to 22px computed font size (16 * 440/320), since the perpendicular writing-mode compared to its containing block causes this to be a new cluster. Unfortunately due to <a href="https://bugs.webkit.org/show_bug.cgi?id=96557">http://webkit.org/b/96557</a> the height:440px is incorrectly interpreted as constraining the logicalHeight, so it doesn't get autosized. > +</div> You forgot to set the font-size here.
Comment on attachment 164951 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164951&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:85 >> +{ > > ASSERT(isAutosizingCluster(cluster)); Done. >> Source/WebCore/rendering/TextAutosizer.cpp:93 >> + float commonAncestorWidth = lowestCommonAncestor->contentLogicalWidth(); > > That should be easy to do by adding back the right scrollbar width. You can add an helper on RenderBox to do that. Yeah, but I am (or will be) dealing with the width of the lowestCommonAncestor, and it's usually some ancestor of the LCA (e.g. RenderView) that has the actual scrollbar. I've removed this FIXME, since I can't see a reasonable way of implementing it, and it's not actually relevant in most contexts where Text Autosizing would be used since they generally use overlay scrollbars which don't affect the width of the contents. >> Source/WebCore/rendering/TextAutosizer.cpp:98 >> + int logicalLayoutWidth = cluster->isHorizontalWritingMode() ? m_minLayoutSize.width() : m_minLayoutSize.height(); > > Nit: This could be an helper function on IntSize akin to FractionalLayoutBoxExtent Discussed offline, there didn't seem to be a neat way of doing this... >> Source/WebCore/rendering/TextAutosizer.cpp:126 >> { > > ASSERT(isAutosizingContainer(container)); Done. >> Source/WebCore/rendering/TextAutosizer.cpp:136 >> // FIXME: Increase list marker size proportionately. > > Does this comment still applies (cf comment in isAutosizingContainer)? Yes. This comment is about increasing the size of list *markers* e.g. bullets, which aren't currently autosized, but should be autosized in proportion to their <li>. >> Source/WebCore/rendering/TextAutosizer.cpp:142 >> + processContainer(multiplier, descendantBlock, descendant); > > Let's use descendantBlock everywhere here instead of mixing descendant and descendantBlock which makes it look like we are passing different pointers. Done (yeah, was debating this). >> Source/WebCore/rendering/TextAutosizer.cpp:230 >> +{ > > ASSERT(isAutosizingCluster(lowestCommonAncestor); But it's not a cluster, in the general case. I think the only strict requirement is that it be a RenderObject (so we can traverse its descendants). >> Source/WebCore/rendering/TextAutosizer.cpp:234 >> + // than that, so we're overestimating their contribution to the linecount). > > Would be good to explain 'why' you are not auto-sizing anything less than 4 lines here instead of in the ChangeLog. Done. >> Source/WebCore/rendering/TextAutosizer.h:77 >> + IntSize m_minLayoutSize; > > Those 2 would be better in a structure that we pass around. As you don't need them after a |processSubtree| phase, it would ensure that no one tries to query them outside it. I wonder if we could throw in the other arguments too (like the |container| and the |multiplier|), feel free to push back on this idea. Done (couldn't throw in other arguments, as they need to be mutated too often). >> LayoutTests/fast/text-autosizing/clusters-sufficient-width-expected.html:32 >> +</div> > > You forgot to set the font-size here. No, see "Unfortunately ... doesn't get autosized".
Created attachment 164997 [details] Patch Address jchaffraix's latest review comments.
Comment on attachment 164997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164997&action=review r=me, please update the ChangeLog before landing. > Source/WebCore/ChangeLog:40 > + - Store windowSize and minLayoutSize in member variables rather than > + passing them as parameters. That was changed. > Source/WebCore/ChangeLog:105 > + - Added m_windowSize and m_minLayoutSize members to cache the > + values computed in processSubtree (hence avoid passing them as > + parameters to every call to processCluster/processContainer). Ditto.
Created attachment 165004 [details] Patch for landing Updated ChangeLog.
Comment on attachment 165004 [details] Patch for landing Clearing flags on attachment: 165004 Committed r129195: <http://trac.webkit.org/changeset/129195>
All reviewed patches have been landed. Closing bug.