[webkit-reviews] review requested: [Bug 53642] pop-up button text not rendered correctly according to its direction in <option> : [Attachment 81542] patch w/ layout test

[bugzilla-daemon at webkit.org]

Xiaomei Ji <xji at chromium.org> has asked  for review:
Bug 53642: pop-up button text not rendered correctly according to its direction
in <option>
https://bugs.webkit.org/show_bug.cgi?id=53642

Attachment 81542: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=81542&action=review

------- Additional Comments from Xiaomei Ji <xji at chromium.org>
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.