Bug 106792

Created attachment 182571 [details] Patch

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?

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

Created attachment 182968 [details] Patch

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.

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

Created attachment 182987 [details] Patch

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.

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

Created attachment 183004 [details] Patch

Comment on attachment 183004 [details] Patch Looks good to me. Kenneth/Julien, what do you think?

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?

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')

Created attachment 183479 [details] Patch

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)

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

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

Created attachment 183500 [details] Patch

You cannot add r+ yourself, you need to add the reviewer in the files directly and just request cq

Created attachment 183503 [details] Patch

Comment on attachment 183503 [details] Patch just set r as empty in the future if the reviewer field is filled in

Comment on attachment 183503 [details] Patch Clearing flags on attachment: 183503 Committed r140193: <http://trac.webkit.org/changeset/140193>

All reviewed patches have been landed. Closing bug.