Bug 53642 – pop-up button text not rendered correctly according to its direction in <option>

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Xiaomei Ji]

After r76983 (http://trac.webkit.org/changeset/76983), button text's directionality might change even if the texts are the same logically.
So, need to re-layout the text even if the text remain the same logically.

Steps to reproduce the bug:
1. open Source/WebCore/manual-tests/pop-up-alignment-and-direction.html
2. click to open the pop-up menu and select the 2nd item (do not click anywhere and keep the selection box highlighted)
3. the item appeared in the selected box as LTR (which is wrong)
4. click in an empty space, the item rendered in the selected box as RTL (which is correct).

[Xiaomei Ji]

Created attachment 81007 [details]
patch

Part of the code I do not understand is the difference between m_innerBlock's style and m_buttonText's style. They are setting different values in the code. And I do now how when m_button's style will take effect.

Comment on attachment 81007 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81007&action=review

Good catch! I thought I had this covered by the change to RenderStyle::diff(), but of course I didn’t, because adjustInnerStyle() just pokes the style directly.

> Source/WebCore/rendering/RenderMenuList.cpp:216
>              m_buttonText->setText(s.impl());
> +            if (!m_buttonText->selfNeedsLayout() && document()->page()->chrome()->selectItemAlignmentFollowsMenuWritingDirection() && m_optionStyle
> +                && (m_optionStyle->direction() != m_innerBlock->style()->direction() || m_optionStyle->unicodeBidi() != m_innerBlock->style()->unicodeBidi()))
> +                m_buttonText->setNeedsLayout(true);

I don’t think this careful checking of the conditions is really necessary, so you could just accomplish this by calling setText(s.imply(), true) above. But really the right thing to do is to do a RenderStyle::diff() in adjustInnerStyle() and if you get a layout difference, call setNeedsLayoutAndPrefWidthsRecalc().

[Xiaomei Ji]

Created attachment 81017 [details]
patch

Thanks for the quick review and feedback!
Update patch per suggestion.

And I have 2 questions:
1. what is the difference between m_innerBlock's style and m_buttonText's style?  They are setting different values in the code. when m_button's style takes effect?
2. anyway to do automatic test? I am in the process of implementing the same for chromium port and found out another regression on a <select> related manual test I added.

Comment on attachment 81017 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81017&action=review

> Source/WebCore/rendering/RenderMenuList.cpp:106
> +        StyleDifference diff = StyleDifferenceEqual;
> +        unsigned contextSensitiveProperties = ContextSensitivePropertyNone;
> +        diff = m_optionStyle->diff(innerStyle, contextSensitiveProperties);
> +        if (diff == StyleDifferenceLayout)
> +            m_innerBlock->setNeedsLayoutAndPrefWidthsRecalc();
> +        

I think I have led you down the wrong path. My apologies. Comparing innerStyle to m_optionStyle is likely to always result in a layout difference, since the inner style mostly inherits from the button, and the option style is quite different (for one, I think it has display: none). I initially intended for the comparison to be between the inner style as it was and the adjusted inner style, but since we don’t clone the style but rather just modify it in-place, we can’t make this comparison. I still think this is the right place to decide whether to call setNeedsLayoutAndPrefWidthsRecalc(), but the condition should be more along the lines of what you initially had, i.e. if the alignment, the direction, or the unicode-bidi changed, then layout is needed.

Sorry about misleading you.

(In reply to comment #3)
> Created an attachment (id=81017) [details]
> patch
> 
> Thanks for the quick review and feedback!
> Update patch per suggestion.
> 
> And I have 2 questions:
> 1. what is the difference between m_innerBlock's style and m_buttonText's style?  They are setting different values in the code. when m_button's style takes effect?

Hm… that’s a good question. I can see how the styles could get out of sync, which might actually be a problem for direction and unicode-bidi since some code probably looks at the RenderText’s style directly.

> 2. anyway to do automatic test? I am in the process of implementing the same for chromium port and found out another regression on a <select> related manual test I added.

Would something like

var selectElement = document.getElementById("…");
document.body.offsetTop; // Force layout
selectElement.selectedIndex = 1;

work? You could even use multiple select elements, each with a different combination of initial and final selected index and do the whole thing in one test.

[Xiaomei Ji]

Created attachment 81542 [details]
patch w/ layout test

Thanks for the review and feedback!

(In reply to comment #4)
> (From update of attachment 81017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81017&action=review
> 
> > Source/WebCore/rendering/RenderMenuList.cpp:106
> > +        StyleDifference diff = StyleDifferenceEqual;
> > +        unsigned contextSensitiveProperties = ContextSensitivePropertyNone;
> > +        diff = m_optionStyle->diff(innerStyle, contextSensitiveProperties);
> > +        if (diff == StyleDifferenceLayout)
> > +            m_innerBlock->setNeedsLayoutAndPrefWidthsRecalc();
> > +        
> 
> I think I have led you down the wrong path. My apologies. Comparing innerStyle to m_optionStyle is likely to always result in a layout difference, since the inner style mostly inherits from the button, and the option style is quite different (for one, I think it has display: none). I initially intended for the comparison to be between the inner style as it was and the adjusted inner style, but since we don’t clone the style but rather just modify it in-place, we can’t make this comparison. I still think this is the right place to decide whether to call setNeedsLayoutAndPrefWidthsRecalc(), but the condition should be more along the lines of what you initially had, i.e. if the alignment, the direction, or the unicode-bidi changed, then layout is needed.


Seems like we do not need to check for alignment change here. It is derived from style()->direction(), and the code should already handle re-layout on its change. Ran manual test pop-up-alignment-and-direction.html verified.

> 
> Sorry about misleading you.

(In reply to comment #5)
> (In reply to comment #3)
> > Created an attachment (id=81017) [details] [details]
> > patch
> > 
> > Thanks for the quick review and feedback!
> > Update patch per suggestion.
> > 
> > And I have 2 questions:
> > 1. what is the difference between m_innerBlock's style and m_buttonText's style?  They are setting different values in the code. when m_button's style takes effect?
> 
> Hm… that’s a good question. I can see how the styles could get out of sync, which might actually be a problem for direction and unicode-bidi since some code probably looks at the RenderText’s style directly.

Filed bug 53944.


> 
> > 2. anyway to do automatic test? I am in the process of implementing the same for chromium port and found out another regression on a <select> related manual test I added.
> 
> Would something like
> 
> var selectElement = document.getElementById("…");
> document.body.offsetTop; // Force layout
> selectElement.selectedIndex = 1;
> 
> work? You could even use multiple select elements, each with a different combination of initial and final selected index and do the whole thing in one test.

Thanks! "document.body.offsetTop;"  did the trick.

[Xiaomei Ji]

committed r77952 <http://trac.webkit.org/changeset/77952>

[Martin Robinson]

(In reply to comment #7)
> committed r77952 <http://trac.webkit.org/changeset/77952>

Instead of just skipping these tests up-front on GTK+ and Qt, I recommend first trying to contact some people working on those ports to generate results, or even just using webkit-patch rebaseline. Since you wrote the test, you're in a better position to interpret the results.