Text Autosizing divides the page into clusters (see bug 97025). The contents of each cluster is autosized according to the width of the cluster. But sometimes the actual text within a cluster only occupies part of the width of the cluster. A particularly frequent example, is that the RenderView itself is often the cluster that contains much of the text on a page; yet many pages have borders or a max-width assigned, hence the actual text content within that page is significantly narrower than the RenderView. Thus you can zoom in on just the text content without needing to pan horizontally, so there is no need to multiply the font size by as much. To solve this, when calculating the width of a cluster, we should first determine 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 of the width of the actual cluster node.
Created attachment 176221 [details] Patch
Comment on attachment 176221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176221&action=review Please fix style first, so style comments for now > Source/WebCore/rendering/TextAutosizer.cpp:345 > + blockWithAllText = lower->containingBlock(); > + ASSERT(blockWithAllText->isDescendantOf(cluster)); > + return blockWithAllText; I would add a newline after the if-else block and one before return > Source/WebCore/rendering/TextAutosizer.cpp:351 > + return findFinalTextLeafNotInCluster( > + parent, depth, &RenderObject::firstChild, &RenderObject::nextSibling); put that on one line, following normal coding style > Source/WebCore/rendering/TextAutosizer.cpp:357 > + return findFinalTextLeafNotInCluster( > + parent, depth, &RenderObject::lastChild, &RenderObject::previousSibling); same > Source/WebCore/rendering/TextAutosizer.cpp:364 > +RenderObject* TextAutosizer::findFinalTextLeafNotInCluster( > + RenderObject* parent, > + size_t& depth, > + RenderObject* (RenderObject::*getFirstChildMethod)() const, > + RenderObject* (RenderObject::*getNextSiblingMethod)() const) this HAS to be on one line according to style > Source/WebCore/rendering/TextAutosizer.cpp:375 > + RenderObject* leaf = findFinalTextLeafNotInCluster( > + child, depth, getFirstChildMethod, getNextSiblingMethod); same > Source/WebCore/rendering/TextAutosizer.h:88 > + static RenderObject* findFirstTextLeafNotInCluster( > + RenderObject* parent, size_t& depth); > + // Finds the last leaf text node child that doesn't belong to any cluster. > + static RenderObject* findLastTextLeafNotInCluster( > + RenderObject* parent, size_t& depth); > + static RenderObject* findFinalTextLeafNotInCluster( > + RenderObject* parent, > + size_t& depth, > + RenderObject* (RenderObject::*getFirstChildMethod)() const, > + RenderObject* (RenderObject::*getNextSiblingMethod)() const); Wrong coding style. These should be on one line.
Created attachment 176226 [details] Patch
Comment on attachment 176221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176221&action=review Fixed. >> Source/WebCore/rendering/TextAutosizer.cpp:345 >> + return blockWithAllText; > > I would add a newline after the if-else block and one before return Done. >> Source/WebCore/rendering/TextAutosizer.cpp:351 >> + parent, depth, &RenderObject::firstChild, &RenderObject::nextSibling); > > put that on one line, following normal coding style Done. >> Source/WebCore/rendering/TextAutosizer.cpp:357 >> + parent, depth, &RenderObject::lastChild, &RenderObject::previousSibling); > > same Done. >> Source/WebCore/rendering/TextAutosizer.cpp:364 >> + RenderObject* (RenderObject::*getNextSiblingMethod)() const) > > this HAS to be on one line according to style Done. >> Source/WebCore/rendering/TextAutosizer.cpp:375 >> + child, depth, getFirstChildMethod, getNextSiblingMethod); > > same Done. >> Source/WebCore/rendering/TextAutosizer.h:88 >> + RenderObject* (RenderObject::*getNextSiblingMethod)() const); > > Wrong coding style. These should be on one line. Done.
Comment on attachment 176221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176221&action=review > Source/WebCore/rendering/TextAutosizer.cpp:302 > +RenderBlock* TextAutosizer::findDeepestBlockWithAllText(RenderBlock* cluster) ContainingAllText ? > Source/WebCore/rendering/TextAutosizer.cpp:319 > + // Determine which node is lower. > + RenderObject* lower = firstTextLeaf; > + RenderObject* higher = lastTextLeaf; I am not sure the lower,higher terminology is that clear
Created attachment 176242 [details] Patch
Comment on attachment 176221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176221&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:302 >> +RenderBlock* TextAutosizer::findDeepestBlockWithAllText(RenderBlock* cluster) > > ContainingAllText ? Done. >> Source/WebCore/rendering/TextAutosizer.cpp:319 >> + RenderObject* higher = lastTextLeaf; > > I am not sure the lower,higher terminology is that clear Changed to deep/shallow
did John look at this yet?
(In reply to comment #8) > did John look at this yet? I discussed this several times during implementation but I'm still reviewing the code itself. Thanks for responding quickly as always!
Comment on attachment 176242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176242&action=review General approach looks great; got some minor nits on the code. I haven't yet reviewed the tests - I'm doing that now. > Source/WebCore/rendering/TextAutosizer.cpp:98 > + // RenderView,instead of just taking the width of |cluster| we should find Nits: s/RenderView,instead/RenderView, instead/ s/we should find/we find/ > Source/WebCore/rendering/TextAutosizer.cpp:-105 > - if (clusterShouldBeAutosized(lowestCommonAncestor, commonAncestorWidth)) { Why did you change this? Presumably all the text within the cluster will be in the subtree descending from the LCA, so when we traverse the cluster looking for text we may as well only traverse that subtree? > Source/WebCore/rendering/TextAutosizer.cpp:-229 > - ASSERT(isAutosizingContainer(renderer)); What was the reason for changing this to a check? If this is just for the sake of findFinalTextLeafNotInCluster, I've left a comment below showing how you could tweak that so this ASSERT remains true. > Source/WebCore/rendering/TextAutosizer.cpp:305 > + if (cluster->isEmpty()) This isEmpty check seems unnecessary, since the next thing you do is call findFirstTextLeafNotInCluster which immediately returns 0 anyway (also causing you to return the cluster). > Source/WebCore/rendering/TextAutosizer.cpp:322 > + if (firstDepth < lastDepth) { Rather than determining which node is deeper, it might be simpler to just repeat the equalizing depths code, e.g.: // Equalize the depths if necessary. while (firstDepth > lastDepth) { firstTextLeaf = firstTextLeaf->parent(); --firstDepth; } while (firstDepth < lastDepth) { lastTextLeaf = lastTextLeaf->parent(); --lastDepth; } You'd save 6 lines of code (and a dependency on std::swap) ;) If you prefer not to do this, would you mind adding a #include <algorithm> in the spirit of http://code.google.com/p/include-what-you-use/wiki/WhyIWYU ? > Source/WebCore/rendering/TextAutosizer.cpp:345 > + ASSERT(blockContainingAllText->isDescendantOf(cluster)); Ultra minor nit: you could save a few lines by reordering this section: ASSERT(deepNode->isDescendantOf(cluster)); if (deepNode->isRenderBlock()) return toRenderBlock(deepNode); else return deepNode->containingBlock(); But I don't mind if you think it was clearer the other way... > Source/WebCore/rendering/TextAutosizer.cpp:360 > +const RenderObject* TextAutosizer::findFinalTextLeafNotInCluster(const RenderObject* parent, size_t& depth, RenderObject* (RenderObject::*getFirstChildMethod)() const, RenderObject* (RenderObject::*getNextSiblingMethod)() const) The function pointers are a neat solution. But I wonder if it might be easier to read without them. Something like: const RenderObject* TextAutosizer::findFirstTextLeafNotInCluster(const RenderObject* parent, size_t& depth, bool firstNotLast) { ... const RenderObject* child = firstNotLast ? parent->firstChild() : parent->lastChild(); while (child) { ... child = firstNotLast ? child->nextSibling() : child->previousSibling() } .... } > Source/WebCore/rendering/TextAutosizer.cpp:367 > + if (child->isRenderBlock() && isAutosizingCluster(toRenderBlock(child))) You can use: if (isAutosizingContainer(child) && isAutosizingCluster(toRenderBlock(child))) to avoid changing the ASSERT in isAutosizingCluster. > LayoutTests/ChangeLog:9 > + RenderView,instead of just taking the width of |cluster| we should find Nit: s/RenderView,instead/RenderView, instead/
Created attachment 176278 [details] Patch
Comment on attachment 176242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176242&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:98 >> + // RenderView,instead of just taking the width of |cluster| we should find > > Nits: > s/RenderView,instead/RenderView, instead/ > s/we should find/we find/ Done. >> Source/WebCore/rendering/TextAutosizer.cpp:-105 >> - if (clusterShouldBeAutosized(lowestCommonAncestor, commonAncestorWidth)) { > > Why did you change this? Presumably all the text within the cluster will be in the subtree descending from the LCA, so when we traverse the cluster looking for text we may as well only traverse that subtree? I misinterpreted the code here. >> Source/WebCore/rendering/TextAutosizer.cpp:-229 >> - ASSERT(isAutosizingContainer(renderer)); > > What was the reason for changing this to a check? If this is just for the sake of findFinalTextLeafNotInCluster, I've left a comment below showing how you could tweak that so this ASSERT remains true. The reason was also for the sake of less coupling (one always have to check for autosizing container before checking for cluster). Since in some places it actually saves a check for container, changed it back. >> Source/WebCore/rendering/TextAutosizer.cpp:305 >> + if (cluster->isEmpty()) > > This isEmpty check seems unnecessary, since the next thing you do is call findFirstTextLeafNotInCluster which immediately returns 0 anyway (also causing you to return the cluster). Removed. >> Source/WebCore/rendering/TextAutosizer.cpp:322 >> + if (firstDepth < lastDepth) { > > Rather than determining which node is deeper, it might be simpler to just repeat the equalizing depths code, e.g.: > > // Equalize the depths if necessary. > while (firstDepth > lastDepth) { > firstTextLeaf = firstTextLeaf->parent(); > --firstDepth; > } > while (firstDepth < lastDepth) { > lastTextLeaf = lastTextLeaf->parent(); > --lastDepth; > } > > You'd save 6 lines of code (and a dependency on std::swap) ;) > If you prefer not to do this, would you mind adding a #include <algorithm> in the spirit of http://code.google.com/p/include-what-you-use/wiki/WhyIWYU ? Makes things shorter though less obvious that only one loop will actually execute (added a comment for that). <algorithm> still needed since std::min and std::max are used. >> Source/WebCore/rendering/TextAutosizer.cpp:345 >> + ASSERT(blockContainingAllText->isDescendantOf(cluster)); > > Ultra minor nit: you could save a few lines by reordering this section: > > ASSERT(deepNode->isDescendantOf(cluster)); > if (deepNode->isRenderBlock()) > return toRenderBlock(deepNode); > else > return deepNode->containingBlock(); > > But I don't mind if you think it was clearer the other way... The assertion was to catch an case when containingBlock goes beyond the cluster, hence it's called after we may use this method. I removed it completely (else return then triggered a warning so else went away as well). >> Source/WebCore/rendering/TextAutosizer.cpp:360 >> +const RenderObject* TextAutosizer::findFinalTextLeafNotInCluster(const RenderObject* parent, size_t& depth, RenderObject* (RenderObject::*getFirstChildMethod)() const, RenderObject* (RenderObject::*getNextSiblingMethod)() const) > > The function pointers are a neat solution. But I wonder if it might be easier to read without them. Something like: > > const RenderObject* TextAutosizer::findFirstTextLeafNotInCluster(const RenderObject* parent, size_t& depth, bool firstNotLast) > { > ... > const RenderObject* child = firstNotLast ? parent->firstChild() : parent->lastChild(); > while (child) { > ... > child = firstNotLast ? child->nextSibling() : child->previousSibling() > } > .... > } Done. I used an enum instead of bool in an attempt to follow the style-guide: http://www.webkit.org/coding/coding-style.html#names-enum-to-bool I also used wrapper functions instead of ternary operator. >> Source/WebCore/rendering/TextAutosizer.cpp:367 >> + if (child->isRenderBlock() && isAutosizingCluster(toRenderBlock(child))) > > You can use: > > if (isAutosizingContainer(child) && isAutosizingCluster(toRenderBlock(child))) > > to avoid changing the ASSERT in isAutosizingCluster. Done. >> LayoutTests/ChangeLog:9 >> + RenderView,instead of just taking the width of |cluster| we should find > > Nit: s/RenderView,instead/RenderView, instead/ Done.
Created attachment 176280 [details] Patch
Created attachment 176288 [details] Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=176242&action=review Some minor nits on the comments too. Thanks! > LayoutTests/fast/text-autosizing/cluster-wide-in-narrow.html:29 > + <div style="width: 600px"> I'm not quite sure why you're making this change now, since this patch doesn't affect this test (though I suspect your follow-on patch will :) > LayoutTests/fast/text-autosizing/cluster-with-narrow-lca.html:23 > +<div style="width: 400px" > Nit: s/" >/">/ > LayoutTests/fast/text-autosizing/cluster-with-narrow-lca.html:25 > + This text should be autosized to just 20px computed font size (16 * 400/320), since the width of the least common ancestor is used for multiplier calculation. It's not quite clear what least common ancestor refers to; perhaps "the width of the least common ancestor of the cluster's text descendants" would be clearer? > LayoutTests/fast/text-autosizing/cluster-with-wide-lca.html:25 > + This text should be autosized to 40px computed font size (16 * 800/320). Please explain briefly why (ditto below). > LayoutTests/fast/text-autosizing/em-margin-border-padding.html:24 > + This text should get autosized to 35.2px computed font size (16 * 704/320), but the 1em margins, borders and padding should remain just 16px. While debatable, this generally looks more consistent than having them be scaled.<br> Thanks for adding the equation here. It's mildly unfortunate that we have a fractional size though; if you can think of an easy way of tweaking the numbers so it comes out integral that would be nice (as it would reduce the likelihood of rounding bugs on different platforms). Though when changing this, please make sure the multiplier remains expressible in a small finite number of decimal places (not e.g. 2.3333333333). > LayoutTests/fast/text-autosizing/wide-child-expected.html:15 > + This text should not be autosized. Nit: please add ", as this div is the lowest common ancestor of the root cluster, and this div is narrow" or similar. Also, a subtle gotcha with these tests is that Text Autosizing only autosizes clusters with enough text, so whenever you're asserting that something shouldn't be autosized, it's best for that something to have a decent amount of text so prevent passing the test for the wrong reason (i.e. accidentally passing the test because there wasn't enough text). So you might want to add a Lorem...aliqua line underneath this one (after a <br>). > LayoutTests/fast/text-autosizing/wide-child-expected.html:17 > + This text should not be autosized as well since its width doesn't affect the width of the parent block. Please explain why the parent block is significant. > LayoutTests/fast/text-autosizing/wide-in-narrow-overflow-scroll.html:26 > + This text should be autosized to 20px computed font size (16 * 400/320), since its scroll width is 400px.<br> Could you clarify this. Something like "since this is part of the root cluster, whose text descendants are all contained within the 400px wide grandparent of this div."
Since I failed to find these last comments in patch, answering in plain text: > > LayoutTests/fast/text-autosizing/cluster-wide-in-narrow.html:29 > > + <div style="width: 600px"> > > I'm not quite sure why you're making this change now, since this patch doesn't affect this test (though I suspect your follow-on patch will :) Reverted. > > > LayoutTests/fast/text-autosizing/cluster-with-narrow-lca.html:23 > > +<div style="width: 400px" > > > Nit: s/" >/">/ Done. > > > LayoutTests/fast/text-autosizing/cluster-with-narrow-lca.html:25 > > + This text should be autosized to just 20px computed font size (16 * 400/320), since the width of the least common ancestor is used for multiplier calculation. > > It's not quite clear what least common ancestor refers to; perhaps "the width of the least common ancestor of the cluster's text descendants" would be clearer? Agree. Done. > > > LayoutTests/fast/text-autosizing/cluster-with-wide-lca.html:25 > > + This text should be autosized to 40px computed font size (16 * 800/320). > > Please explain briefly why (ditto below). Done. > > > LayoutTests/fast/text-autosizing/em-margin-border-padding.html:24 > > + This text should get autosized to 35.2px computed font size (16 * 704/320), but the 1em margins, borders and padding should remain just 16px. While debatable, this generally looks more consistent than having them be scaled.<br> > > Thanks for adding the equation here. It's mildly unfortunate that we have a fractional size though; if you can think of an easy way of tweaking the numbers so it comes out integral that would be nice (as it would reduce the likelihood of rounding bugs on different platforms). Though when changing this, please make sure the multiplier remains expressible in a small finite number of decimal places (not e.g. 2.3333333333). Increased body width to 896 so that div width is 800. > > > LayoutTests/fast/text-autosizing/wide-child-expected.html:15 > > + This text should not be autosized. > > Nit: please add ", as this div is the lowest common ancestor of the root cluster, and this div is narrow" or similar. Also, a subtle gotcha with these tests is that Text Autosizing only autosizes clusters with enough text, so whenever you're asserting that something shouldn't be autosized, it's best for that something to have a decent amount of text so prevent passing the test for the wrong reason (i.e. accidentally passing the test because there wasn't enough text). So you might want to add a Lorem...aliqua line underneath this one (after a <br>). Done. There actually was additional text, but after child div. Moved it to the front. > > > LayoutTests/fast/text-autosizing/wide-child-expected.html:17 > > + This text should not be autosized as well since its width doesn't affect the width of the parent block. > > Please explain why the parent block is significant. Done. > > > LayoutTests/fast/text-autosizing/wide-in-narrow-overflow-scroll.html:26 > > + This text should be autosized to 20px computed font size (16 * 400/320), since its scroll width is 400px.<br> > > Could you clarify this. Something like "since this is part of the root cluster, whose text descendants are all contained within the 400px wide grandparent of this div." Done.
Created attachment 176342 [details] Patch
Comment on attachment 176342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176342&action=review Looking good. Final minor nits: > Source/WebCore/ChangeLog:21 > + (WebCore::TextAutosizer::isAutosizingCluster): replaced assertion for being autosizing container with a check This comment is stale. > Source/WebCore/rendering/TextAutosizer.cpp:337 > + // We assume that containingBlock() can't skip the cluster, since as of now (11/27/2012) it skips I just realized that this assumption isn't currently true - my bad for telling you it was earlier! For this to hold, you'll need to adjust isAutosizingContainer so that it returns true if renderer->isRenderBlock() && renderer->isOutOfFlowPositioned(), even if it is a list item. Nit: I'd reword this comment to: // containingBlock() should never leave the cluster, since it only skips ancestors when finding the // container of position:absolute/fixed blocks, and those cannot exist within a cluster as // isAutosizingCluster would have made them into their own independent cluster. > Source/WebCore/rendering/TextAutosizer.cpp:374 > +const RenderObject* TextAutosizer::firstChild(const RenderObject* object, TraversalDirection direction) These methods are confusingly named - it sounds like it returns the first child of the TextAutosizer (whatever that would mean). Nit: They're also kinda verbose; it would probably be sufficient to do: const RenderObject* child = direction == FirstToLast ? parent->firstChild() : parent->lastChild(); (since the enum lives in TextAutosizer.h, I wouldn't worry about people adding new entries to the enum without taking into account the uses in TextAutosizer.cpp)
Comment on attachment 176342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176342&action=review > LayoutTests/fast/text-autosizing/cluster-with-narrow-lca-and-cluster-expected.html:14 > +<div style="width: 400px" > Nit: please don't leave spaces between " and >, here and in your other test files (probably best to grep for /" >$/). > LayoutTests/fast/text-autosizing/cluster-with-narrow-lca-and-cluster-expected.html:25 > + Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. Nit: please indent this the same as "This ...". And you probably want a <br> after "another cluster."
Created attachment 176512 [details] Patch
Created attachment 176523 [details] Patch
Comment on attachment 176342 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176342&action=review >> Source/WebCore/ChangeLog:21 >> + (WebCore::TextAutosizer::isAutosizingCluster): replaced assertion for being autosizing container with a check > > This comment is stale. Thanks! Fixed. >> Source/WebCore/rendering/TextAutosizer.cpp:337 >> + // We assume that containingBlock() can't skip the cluster, since as of now (11/27/2012) it skips > > I just realized that this assumption isn't currently true - my bad for telling you it was earlier! For this to hold, you'll need to adjust isAutosizingContainer so that it returns true if renderer->isRenderBlock() && renderer->isOutOfFlowPositioned(), even if it is a list item. > > Nit: I'd reword this comment to: > > // containingBlock() should never leave the cluster, since it only skips ancestors when finding the > // container of position:absolute/fixed blocks, and those cannot exist within a cluster as > // isAutosizingCluster would have made them into their own independent cluster. Fixed. Added a test for this case. >> Source/WebCore/rendering/TextAutosizer.cpp:374 >> +const RenderObject* TextAutosizer::firstChild(const RenderObject* object, TraversalDirection direction) > > These methods are confusingly named - it sounds like it returns the first child of the TextAutosizer (whatever that would mean). > > Nit: They're also kinda verbose; it would probably be sufficient to do: > > const RenderObject* child = direction == FirstToLast ? parent->firstChild() : parent->lastChild(); > > (since the enum lives in TextAutosizer.h, I wouldn't worry about people adding new entries to the enum without taking into account the uses in TextAutosizer.cpp) I guess I kept trying to keep this optimized (i.e. with __builtin_unreachable or __assume). Inlined them in the calling function. >> LayoutTests/fast/text-autosizing/cluster-with-narrow-lca-and-cluster-expected.html:14 >> +<div style="width: 400px" > > > Nit: please don't leave spaces between " and >, here and in your other test files (probably best to grep for /" >$/). Done. >> LayoutTests/fast/text-autosizing/cluster-with-narrow-lca-and-cluster-expected.html:25 >> + Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. > > Nit: please indent this the same as "This ...". And you probably want a <br> after "another cluster." Done.
Comment on attachment 176523 [details] Patch Patch looks good to me. Kenneth/Julien, is this ready for r+ ?
Comment on attachment 176523 [details] Patch Could someone cq+ this, please?
Comment on attachment 176523 [details] Patch Clearing flags on attachment: 176523 Committed r136047: <http://trac.webkit.org/changeset/136047>
All reviewed patches have been landed. Closing bug.