Bug 112395 – Convert <select> to new-flexbox

• Component: New Bugs
• Status: RESOLVED FIXED

[Christian Biesinger]

Convert <select> to new-flexbox

[Christian Biesinger]

Created attachment 193206 [details]
Patch

[Ojan Vafai]

Comment on attachment 193206 [details]
Patch

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

> Source/WebCore/rendering/RenderMenuList.h:123
> +    virtual int baselinePosition(FontBaseline baseline, bool firstLine, LineDirectionMode direction, LinePositionMode position) const OVERRIDE
> +    {
> +        return RenderBlock::baselinePosition(baseline, firstLine, direction, position);
> +    }
> +    virtual int firstLineBoxBaseline() const OVERRIDE { return RenderBlock::firstLineBoxBaseline(); }
> +    virtual int inlineBlockBaseline(LineDirectionMode direction) const OVERRIDE { return RenderBlock::inlineBlockBaseline(direction); }

I think this deserves a comment in the code explaining *why* we need to override these. Something like...
// Flexbox defines baselines differently than regular blocks.
// For backwards compatibility, menulists need to do the regular block behavior.

[Christian Biesinger]

Created attachment 193384 [details]
Patch

[WebKit Review Bot]

Comment on attachment 193384 [details]
Patch

Clearing flags on attachment: 193384

Committed r145959: <http://trac.webkit.org/changeset/145959>

[Roger Fong]

> // Flexbox defines baselines differently than regular blocks.
> // For backwards compatibility, menulists need to do the regular block behavior.

Hello, I was hoping I could get some details on what this backwards compatibility thing is for?
I'm run across some bugs that a result of this override.

What I've found is that if ignoreBaseline is RenderBlock::baselinePosition is true, the wrong baselinePosition for the select element is returned.

Thanks!

[Christian Biesinger]

I'm confused. <select> has always used RenderBlock::baselinePosition to calculate its baseline, the override was added so that it keeps doing that. Do you have a specific testcase that now fails but used to work?

[Roger Fong]

Created attachment 202846 [details]
test case

Here it is. When you first load the document everything seems fine.
As soon as you resize the document things go haywire and the red box get's shifted down.

[Christian Biesinger]

Hm, I don't see any change in behavior when I resize my (Chrome) window. But it also looks like this requires use of the deprecated flexbox?

[Roger Fong]

(In reply to comment #8)
> Hm, I don't see any change in behavior when I resize my (Chrome) window. 
Hmm, I can get it to repro if I resize from the left side.
Alternatively if I click on that select element, the box also shifts.

> But it also looks like this requires use of the deprecated flexbox?
Not sure what the difference is. This change moved select from using the DeprecatedFlexbox to the new one right?

[Roger Fong]

I guess what I could do is revert to right before this change and see if that ignoreBaseline is being set to true there as well.

From what I can tell, (layer()->verticalScrollbar() || layer()->scrollYOffset() != 0) are both true for the select element when resizing or clicking the select element. Perhaps, the real problem is that these are being set incorrectly now somehow.

[Roger Fong]

(In reply to comment #5)
> > // Flexbox defines baselines differently than regular blocks.
> > // For backwards compatibility, menulists need to do the regular block behavior.
> 
Hi, is there a test that fails if we don't use these RenderBlock overrides?
If so, could you point me to it so I can take a look at it?
Thanks!

[Christian Biesinger]

fast/forms/basic-selects.html is one of the failing tests. I think there were others but I don't have the full list anymore. But you could just remove it and run the tests yourself.

[Christian Biesinger]

I wonder if this is mac only? I can't reproduce the bug with either resizing or clicking on Linux.

Why not just make the content use new-flexbox? (-webkit-flex instead of -webkit-box, and the related properties)

[Roger Fong]

(In reply to comment #12)
> fast/forms/basic-selects.html is one of the failing tests. I think there were others but I don't have the full list anymore. But you could just remove it and run the tests yourself.

The reason I ask is because after reverting the change I don't actually see any new tests failing. Perhaps it's skipped on  my machine, I'll run it explicitly.

> I wonder if this is mac only? I can't reproduce the bug with either resizing or clicking on Linux.
> Why not just make the content use new-flexbox? (-webkit-flex instead of -webkit-box, and the related properties)
Hrmm strange. 
Well, since the problem does seem to exist for me I'd rather not try to fix it via content changes and try to get to the root of it.

I'll keep digging though, thx for the help!