RESOLVED FIXED 106792
Text Autosizing: don't autosize headers with multiple inline links.
https://bugs.webkit.org/show_bug.cgi?id=106792
Summary Text Autosizing: don't autosize headers with multiple inline links.
timvolodine
Reported 2013-01-14 07:03:01 PST
Don't autosize headers with multiple inline links
Attachments
Patch (8.38 KB, patch)
2013-01-14 07:08 PST, timvolodine
no flags
Patch (11.36 KB, patch)
2013-01-16 05:49 PST, timvolodine
no flags
Patch (11.37 KB, patch)
2013-01-16 08:25 PST, timvolodine
no flags
Patch (11.21 KB, patch)
2013-01-16 10:12 PST, timvolodine
no flags
Patch (14.72 KB, patch)
2013-01-18 09:15 PST, timvolodine
no flags
Patch (15.11 KB, patch)
2013-01-18 10:34 PST, timvolodine
no flags
Patch (15.13 KB, patch)
2013-01-18 10:48 PST, timvolodine
no flags
timvolodine
Comment 1 2013-01-14 07:08:30 PST
John Mellor
Comment 2 2013-01-14 12:23:16 PST
Comment on attachment 182571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182571&action=review Looks pretty good. Commented on the tree traversal and some nits. > Source/WebCore/rendering/TextAutosizer.cpp:298 > +bool TextAutosizer::isHeaderWithLinks(const RenderObject* container) Nit: I'm not sure "header" is a good term for these. They can also occur in the footer for example. How about "isRowOfLinks"? > Source/WebCore/rendering/TextAutosizer.cpp:302 > + // 2. it should contain at least 2 inline link elements Nit: I think s/link/<a>/ would be clearer. > Source/WebCore/rendering/TextAutosizer.cpp:303 > + // 3. it should contain only inline or text elements unless they are children Nit: I don't think you need "or text elements", as text implicitly acts inline. > Source/WebCore/rendering/TextAutosizer.cpp:306 > + int numLinks = 0; Nit: WebKit doesn't like abbreviations. Perhaps "linkCount"? > Source/WebCore/rendering/TextAutosizer.cpp:319 > + const String valueWithoutWhitespaces = currentNode->nodeValue().stripWhiteSpace(); Rather than going via Node, could you not just do toRenderText(current)->text()->stripWhiteSpace()->length() ? > Source/WebCore/rendering/TextAutosizer.cpp:335 > + if (!isHeaderWithLinksCandidate(child, stayWithin, linkCount)) I'm hesitant to add another complicated recursive tree traversal to this file. It seems that what you need is almost exactly the same as nextInPreOrderSkippingDescendantsOfContainers, except that you want to be able to completely skip some arbitrary subtrees of the traversal, which isn't currently possible/practical. This sounds like something that could be useful elsewhere. How about you rename nextInPreOrderSkippingDescendantsOfContainers to nextInPreOrder, and add a new SkipMode parameter, which is an enum that can either be SkipDescendantsOfContainers or SkipAllDescendants? Then you'd be able to do something like: int numLinks = 0; RenderObject* descendant = nextInPreOrder(SkipDescendantsOfContainers, container, container); while (descendant) { ... if (isLink) { numLinks++; descendant = nextInPreOrder(SkipAllDescendants, descendant, container); } else descendant = nextInPreOrder(SkipDescendantsOfContainers, descendant, container); } > LayoutTests/fast/text-autosizing/header-links-autosizing-expected.html:31 > +</div> This </div> doesn't have a matching <div>. Perhaps you didn't mean to include the earlier </div>? > LayoutTests/fast/text-autosizing/header-links-autosizing.html:31 > +<a href="">These</a> Test looks good (apart from the </div>). Could you also include a 2nd test which covers the <li style="display: inline"><a>...</a></li> case?
timvolodine
Comment 3 2013-01-16 05:37:57 PST
Comment on attachment 182571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182571&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:298 >> +bool TextAutosizer::isHeaderWithLinks(const RenderObject* container) > > Nit: I'm not sure "header" is a good term for these. They can also occur in the footer for example. How about "isRowOfLinks"? done >> Source/WebCore/rendering/TextAutosizer.cpp:302 >> + // 2. it should contain at least 2 inline link elements > > Nit: I think s/link/<a>/ would be clearer. done >> Source/WebCore/rendering/TextAutosizer.cpp:303 >> + // 3. it should contain only inline or text elements unless they are children > > Nit: I don't think you need "or text elements", as text implicitly acts inline. now using RenderText and RenderInline to be more specific >> Source/WebCore/rendering/TextAutosizer.cpp:306 >> + int numLinks = 0; > > Nit: WebKit doesn't like abbreviations. Perhaps "linkCount"? done >> Source/WebCore/rendering/TextAutosizer.cpp:319 >> + const String valueWithoutWhitespaces = currentNode->nodeValue().stripWhiteSpace(); > > Rather than going via Node, could you not just do toRenderText(current)->text()->stripWhiteSpace()->length() ? done >> Source/WebCore/rendering/TextAutosizer.cpp:335 >> + if (!isHeaderWithLinksCandidate(child, stayWithin, linkCount)) > > I'm hesitant to add another complicated recursive tree traversal to this file. > > It seems that what you need is almost exactly the same as nextInPreOrderSkippingDescendantsOfContainers, except that you want to be able to completely skip some arbitrary subtrees of the traversal, which isn't currently possible/practical. This sounds like something that could be useful elsewhere. > > How about you rename nextInPreOrderSkippingDescendantsOfContainers to nextInPreOrder, and add a new SkipMode parameter, which is an enum that can either be SkipDescendantsOfContainers or SkipAllDescendants? Then you'd be able to do something like: > > int numLinks = 0; > RenderObject* descendant = nextInPreOrder(SkipDescendantsOfContainers, container, container); > while (descendant) { > ... > if (isLink) { > numLinks++; > descendant = nextInPreOrder(SkipAllDescendants, descendant, container); > } > else > descendant = nextInPreOrder(SkipDescendantsOfContainers, descendant, container); > } done >> LayoutTests/fast/text-autosizing/header-links-autosizing-expected.html:31 >> +</div> > > This </div> doesn't have a matching <div>. Perhaps you didn't mean to include the earlier </div>? done >> LayoutTests/fast/text-autosizing/header-links-autosizing.html:31 >> +<a href="">These</a> > > Test looks good (apart from the </div>). Could you also include a 2nd test which covers the <li style="display: inline"><a>...</a></li> case? done
timvolodine
Comment 4 2013-01-16 05:49:01 PST
John Mellor
Comment 5 2013-01-16 06:41:59 PST
Comment on attachment 182968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182968&action=review Looks good. Some nits. > Source/WebCore/rendering/TextAutosizer.cpp:309 > + // A "header with links" is a container for which holds: s/header with/row of/ > Source/WebCore/rendering/TextAutosizer.cpp:312 > + // 3. it should contain only RenderInline or RenderText elements unless they are children It's confusing to mention RenderInline here, as you can get elements that are inline but aren't RenderInline (such as RenderApplet, RenderListMarker, RenderRubyRun and RenderTableCol). I'd recommend doing s/RenderInline or RenderText/inline/ (whether or not you remove isText below). > Source/WebCore/rendering/TextAutosizer.cpp:321 > + if (!renderer->isText() && !renderer->isInline()) Do you actually need the |!renderer->isText() &&| part? Unless I missed something I think isInline should return true for RenderText elements (or at least for RenderText elements with inline parents, which should be all you encounter here anytime it actually is a row of links). > Source/WebCore/rendering/TextAutosizer.cpp:324 > + const Node* rendererNode = renderer->node(); Nit: probably sufficient to call this just |node|, but I don't really mind... > Source/WebCore/rendering/TextAutosizer.cpp:329 > + renderer = (isLink || isAutosizingContainer(renderer)) ? You can probably simplify this condition using nextInPreOrderSkippingDescendantsOfContainers, e.g.: renderer = isLink ? renderer->nextInPreOrderAfterChildren(container) : nextInPreOrderSkippingDescendantsOfContainers(renderer, container); Or even: if (node && node->isElementNode() && toElement(node)->tagQName() == aTag) { linkCount++; // Skip traversing descendants of the <a>. renderer = renderer->nextInPreOrderAfterChildren(container); } else renderer = nextInPreOrderSkippingDescendantsOfContainers(renderer, container); > LayoutTests/fast/text-autosizing/header-li-links-autosizing-expected.html:38 > + whereas the inline links in the header above and the accompanying time stamp s/and the accompanying time stamp// > LayoutTests/fast/text-autosizing/header-li-links-autosizing.html:26 > +<div> Nit: this <div> is redundant (it's fine for the <ul> to be a direct child of the body). > LayoutTests/fast/text-autosizing/header-links-autosizing-expected.html:18 > + <span style="float: right"> It's a little confusing having this timestamp here, because the test ends up testing that the time stamp doesn't get autosized, but the reason for that happening is different to the reason for the links not getting autosized. Instead, how about something like this: <div> <a>...</a> &gt; <a>...</a> &gt; <a>...</a> &gt; <a>...</a> &gt; <a>...</a> <div> [A bunch of text that should be autosized]. </div> </div> <div> [Some more text that should be autosized]. </div> That way you clearly test the fact that descendant containers are ignored.
timvolodine
Comment 6 2013-01-16 08:20:16 PST
Comment on attachment 182968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182968&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:309 >> + // A "header with links" is a container for which holds: > > s/header with/row of/ done >> Source/WebCore/rendering/TextAutosizer.cpp:312 >> + // 3. it should contain only RenderInline or RenderText elements unless they are children > > It's confusing to mention RenderInline here, as you can get elements that are inline but aren't RenderInline (such as RenderApplet, RenderListMarker, RenderRubyRun and RenderTableCol). I'd recommend doing s/RenderInline or RenderText/inline/ (whether or not you remove isText below). done >> Source/WebCore/rendering/TextAutosizer.cpp:321 >> + if (!renderer->isText() && !renderer->isInline()) > > Do you actually need the |!renderer->isText() &&| part? Unless I missed something I think isInline should return true for RenderText elements (or at least for RenderText elements with inline parents, which should be all you encounter here anytime it actually is a row of links). look like you are right :), removed !isText && test >> Source/WebCore/rendering/TextAutosizer.cpp:324 >> + const Node* rendererNode = renderer->node(); > > Nit: probably sufficient to call this just |node|, but I don't really mind... done >> Source/WebCore/rendering/TextAutosizer.cpp:329 >> + renderer = (isLink || isAutosizingContainer(renderer)) ? > > You can probably simplify this condition using nextInPreOrderSkippingDescendantsOfContainers, e.g.: > > renderer = isLink ? renderer->nextInPreOrderAfterChildren(container) : nextInPreOrderSkippingDescendantsOfContainers(renderer, container); > > Or even: > > if (node && node->isElementNode() && toElement(node)->tagQName() == aTag) { > linkCount++; > // Skip traversing descendants of the <a>. > renderer = renderer->nextInPreOrderAfterChildren(container); > } else > renderer = nextInPreOrderSkippingDescendantsOfContainers(renderer, container); done >> LayoutTests/fast/text-autosizing/header-li-links-autosizing-expected.html:38 >> + whereas the inline links in the header above and the accompanying time stamp > > s/and the accompanying time stamp// done >> LayoutTests/fast/text-autosizing/header-li-links-autosizing.html:26 >> +<div> > > Nit: this <div> is redundant (it's fine for the <ul> to be a direct child of the body). done >> LayoutTests/fast/text-autosizing/header-links-autosizing-expected.html:18 >> + <span style="float: right"> > > It's a little confusing having this timestamp here, because the test ends up testing that the time stamp doesn't get autosized, but the reason for that happening is different to the reason for the links not getting autosized. Instead, how about something like this: > > <div> > <a>...</a> > &gt; > <a>...</a> > &gt; > <a>...</a> > &gt; > <a>...</a> > &gt; > <a>...</a> > <div> > [A bunch of text that should be autosized]. > </div> > </div> > <div> > [Some more text that should be autosized]. > </div> > > That way you clearly test the fact that descendant containers are ignored. done
timvolodine
Comment 7 2013-01-16 08:25:54 PST
John Mellor
Comment 8 2013-01-16 08:56:05 PST
Comment on attachment 182987 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182987&action=review Getting close! Final nits. > Source/WebCore/ChangeLog:4 > + This patch includes code for detecting rows of links typically seen Nit: please put this description after the Reviewed by line, instead of between the bug title and bug url. > Source/WebCore/rendering/TextAutosizer.cpp:311 > + // 2. it should contain at least 2 inline <a> elements s/<a> elements/links/ for consistency with the fact that we're now explicitly checking for links instead of <a> elements (see below). > Source/WebCore/rendering/TextAutosizer.cpp:325 > + if (node && node->isElementNode() && (toElement(node)->tagQName() == aTag)) { Now I think about it, technically just looking for <a> elements isn't quite ideal, as that'll include anchors (i.e. <a> elements with a name attribute but no href attribute, used purely as targets of a #fragment url). Instead you can use renderer->style()->isLink() which is also shorter :) > Source/WebCore/rendering/TextAutosizer.cpp:327 > + // Skip traversing descendants of the <a> element s/<a> element/link./ for consistency with the fact that we're now explicitly checking for links instead of <a> elements. > LayoutTests/fast/text-autosizing/header-links-autosizing-expected.html:35 > + whereas the inline links in the header above should be rendered at their default size. Would be nice to explain why, e.g. s/should be/should be detected as a row of links and hence/ (similarly in other test). It might also be clearer to move this explanation to the paragraph above, to avoid any confusion and clarify that the paragraph above isn't expected to be rendered at default size.
timvolodine
Comment 9 2013-01-16 10:07:03 PST
Comment on attachment 182987 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=182987&action=review >> Source/WebCore/ChangeLog:4 >> + This patch includes code for detecting rows of links typically seen > > Nit: please put this description after the Reviewed by line, instead of between the bug title and bug url. done >> Source/WebCore/rendering/TextAutosizer.cpp:311 >> + // 2. it should contain at least 2 inline <a> elements > > s/<a> elements/links/ for consistency with the fact that we're now explicitly checking for links instead of <a> elements (see below). done >> Source/WebCore/rendering/TextAutosizer.cpp:325 >> + if (node && node->isElementNode() && (toElement(node)->tagQName() == aTag)) { > > Now I think about it, technically just looking for <a> elements isn't quite ideal, as that'll include anchors (i.e. <a> elements with a name attribute but no href attribute, used purely as targets of a #fragment url). Instead you can use renderer->style()->isLink() which is also shorter :) ack, done. >> Source/WebCore/rendering/TextAutosizer.cpp:327 >> + // Skip traversing descendants of the <a> element > > s/<a> element/link./ for consistency with the fact that we're now explicitly checking for links instead of <a> elements. done >> LayoutTests/fast/text-autosizing/header-links-autosizing-expected.html:35 >> + whereas the inline links in the header above should be rendered at their default size. > > Would be nice to explain why, e.g. s/should be/should be detected as a row of links and hence/ (similarly in other test). It might also be clearer to move this explanation to the paragraph above, to avoid any confusion and clarify that the paragraph above isn't expected to be rendered at default size. done
timvolodine
Comment 10 2013-01-16 10:12:01 PST
John Mellor
Comment 11 2013-01-16 10:17:53 PST
Comment on attachment 183004 [details] Patch Looks good to me. Kenneth/Julien, what do you think?
Kenneth Rohde Christiansen
Comment 12 2013-01-16 11:18:22 PST
Comment on attachment 183004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183004&action=review > Source/WebCore/ChangeLog:10 > + This patch includes code for detecting rows of links typically seen > + in headers or footers of webpages. Such rows of links should not be > + autosized. But the code would detect them everywhere on the page right > Source/WebCore/rendering/TextAutosizer.cpp:281 > + if (containerIsRowOfLinks(container)) IsCollectionOfLinks? what about oclumns of links? why is that not a problem?
timvolodine
Comment 13 2013-01-18 09:13:56 PST
Comment on attachment 183004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183004&action=review >> Source/WebCore/ChangeLog:10 >> + autosized. > > But the code would detect them everywhere on the page right yes, and that makes it non-trivial to detect those problematic headers so to speak >> Source/WebCore/rendering/TextAutosizer.cpp:281 >> + if (containerIsRowOfLinks(container)) > > IsCollectionOfLinks? what about oclumns of links? why is that not a problem? It's more specific than just a collection of links. More like a 'row' of links, because we generally don't want to autosize *any* occurrence of collections of links, only the ones that are header-like, because if they are autosized they get wrapped and look really ugly. I will upload a new patch shortly which will be more restrictive about what to call a row of links (i.e. a 'header')
timvolodine
Comment 14 2013-01-18 09:15:03 PST
timvolodine
Comment 15 2013-01-18 09:16:46 PST
A new patch with a more specific definition of what to call a row of links (aka header). We ran a global diff and this fixes most of the 'broken' websites. (also added a font-size specific test)
Kenneth Rohde Christiansen
Comment 16 2013-01-18 09:20:28 PST
Comment on attachment 183479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183479&action=review > Source/WebCore/rendering/TextAutosizer.cpp:310 > + // 1. it should not contain non-link text elements longer than 3 characters it would be nice to know where these heuristics come from, like mention it in the changelog > Source/WebCore/rendering/TextAutosizer.cpp:312 > + // 2. it should contain at least 3 inline links, and all the links should > + // have the same specified font size 2. it should consist of min. 3 inline links using the same specified font size. > Source/WebCore/rendering/TextAutosizer.cpp:335 > + else > + if (matchingFontSize != renderer->style()->specifiedFontSize()) > + return false; > + This else needs braces according to style as it is spanning multiple actual code lines, not logical ones
timvolodine
Comment 17 2013-01-18 09:43:58 PST
Comment on attachment 183479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183479&action=review >> Source/WebCore/rendering/TextAutosizer.cpp:310 >> + // 1. it should not contain non-link text elements longer than 3 characters > > it would be nice to know where these heuristics come from, like mention it in the changelog done >> Source/WebCore/rendering/TextAutosizer.cpp:312 >> + // have the same specified font size > > 2. it should consist of min. 3 inline links using the same specified font size. I guess this formulation could be confusing as it can be interpreted as: contain minimum 3 links of same font size, i.e. 3 links of size 16px and 1 of size 20px would still be ok. >> Source/WebCore/rendering/TextAutosizer.cpp:335 >> + > > This else needs braces according to style as it is spanning multiple actual code lines, not logical ones done
timvolodine
Comment 18 2013-01-18 10:34:24 PST
Kenneth Rohde Christiansen
Comment 19 2013-01-18 10:43:28 PST
You cannot add r+ yourself, you need to add the reviewer in the files directly and just request cq
timvolodine
Comment 20 2013-01-18 10:48:45 PST
Kenneth Rohde Christiansen
Comment 21 2013-01-18 11:41:23 PST
Comment on attachment 183503 [details] Patch just set r as empty in the future if the reviewer field is filled in
WebKit Review Bot
Comment 22 2013-01-18 12:28:04 PST
Comment on attachment 183503 [details] Patch Clearing flags on attachment: 183503 Committed r140193: <http://trac.webkit.org/changeset/140193>
WebKit Review Bot
Comment 23 2013-01-18 12:28:09 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.