[Webkit-unassigned] [Bug 106792] Text Autosizing: don't autosize headers with multiple inline links.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=106792





--- Comment #3 from timvolodine at chromium.org  2013-01-16 05:37:57 PST ---
(From update of attachment 182571)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.