[Webkit-unassigned] [Bug 53642] pop-up button text not rendered correctly according to its direction in <option>

[bugzilla-daemon at webkit.org]

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


Xiaomei Ji <xji at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #81542|                            |review?
               Flag|                            |




--- Comment #6 from Xiaomei Ji <xji at chromium.org>  2011-02-07 15:23:57 PST ---
Created an attachment (id=81542)
 --> (https://bugs.webkit.org/attachment.cgi?id=81542&action=review)
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)
 --> (https://bugs.webkit.org/attachment.cgi?id=81017&action=review) [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.

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