Bug 29167 – REGRESSION (r48064): mint.com loses scrollbars after coming out of edit mode

• Component: New Bugs
• Status: RESOLVED FIXED

[Alice Liu]

Regression from http://trac.webkit.org/changeset/48064

mint.com loses scrollbars after coming out of edit mode

1) log into mint.com
2) click the edit link to edit your accounts (notice the faded-out background loses scrollbars
3) click the close button in the editing overlay

Actual result: the page (which fades in) does not have scrollbars.
Expected Result: for the page to regain scrollbars.  This happens with r48063 and before.

[Mark Rowe (bdash)]

<rdar://problem/7215132>

[Mark Mentovai]

I see this too, and it happens in Chromium as well.

This regressed in bug 28614.  This bug does not occur with the original v2 patch on that bug.  It only occurs with the updated v4 patch, which contained additional fixes for DumpLayoutTree-based layout test regressions that occurred on the Mac only.

I'll take a closer look at this tomorrow.

[Mark Mentovai]

Created attachment 39408 [details]
Possible approach (not for review or checkin)

This was a more conservative approach to the test failures in bug 28614 that led to the creation of the v4 patch.  I recall testing something like this and finding it satisfactory.  It fixes this bug.

I'm not sure that it's the right answer and I haven't done much more testing with this other than what I remember doing with similar logic last week, and now visiting mint.com.  This needs more inspection, I'll look tomorrow.

Comment on attachment 39408 [details]
Possible approach (not for review or checkin)

The patch does fix bug 30517 as well, and is probably the right approach for now. The only thing I would consider changing is not re-introducing two mode variables but just one “can have scrollbars” boolean, but even that is not really necessary and could be left for a later cleanup pass. If you add the test from bug 30517 (or a different test) and add a change log, I could review the patch.

[Mark Mentovai]

OK, I should be able to get to this today or tomorrow.

[Mark Mentovai]

Created attachment 42627 [details]
Track "can have scrollbar" state within FrameView independently of the individual scrollbar states in ScrollView

I started with attachment 39408 [details], which I always thought was more than was necessary to fix this bug, and whittled it away until it contained just the minimum bits needed to get the right behavior.

Now that it's done, I see that it's very similar to what mitz came up with in bug 30517 attachment 42576 [details].

This does take mitz's suggestion to track "can have scrollbars" as a single boolean instead of tracing m_hmode and m_vmode separately.  FrameView shouldn't need to track each scrollbar's on/off/auto mode on its own.  The mode tracking belongs in ScrollView, and I got rid of the duplicate mode tracking in FrameView in bug 28614 and was hoping to not have to bring it back to fix this bug.

I've run this through the WebKit test suite on the Mac, and have fed it to the Chromium layout test trybots (http://codereview.chromium.org/376009).  I've confirmed that it fixes this bug and the attached testcase, and doesn't regress bug 28614.  I haven't tested GarageBand from bug 30517, but the testcase from that bug (and in this patch) does the same thing that mint.com does - it sets overflow:hidden and then clears overflow.

Comment on attachment 42627 [details]
Track "can have scrollbar" state within FrameView independently of the individual scrollbar states in ScrollView

I haven’t tested it yet, but based on my experience with bug 30517, I am afraid that this does not fix the GarageBand issue (even though it fixes the test case), the reason being that without calling initScrollbars() from WebFrameView, the FrameView resets to “can have scroll bars” after navigation (which is the real GarageBand scenario; I haven’t been able to recreate it in a layout test).

I also think that it’s wrong to name the variable m_canScroll. A frame without scroll bars may still be scrolled programmatically, as a side-effect of find-in-page, and by dragging to select.

[Mark Mentovai]

Created attachment 42634 [details]
Track "can have scrollbar" state within FrameView independently of the individual scrollbar states in ScrollView

This one just has the spelling fix (m_canScroll -> m_canHaveScrollbars).

[Mark Mentovai]

Created attachment 42635 [details]
With extra logic in resetScrollbars

This one has a ilttle extra logic in resetScrollbars to more closely match what the first patch on this bug did.

initScrollbars seems like a hack: it was only used by WebFrameView, and only as a one-shot sort of deal.  If this still doesn't work with your GarageBand case, we can go in that direction, but it's a little bit difficult for me without a test case or GarageBand to play with.  You can test these to see if they do (or don't do) the right thing for GarageBand.

This patch does fix the mint.com bug and the testcase, so it's probably worthwhile even if we need to keep hammering away at something for GarageBand.

Confirmed that neither patch resolves the issue with GarageBand. I will try to
whip up a test app that does what GarageBand does. While there may be a cleaner
way to do what the call to initScrollbars() used to do, I would recommend
fixing the regression by taking a step backwards (as in attachment 39408 [details]), then
trying to clean things up after that.

[Mark Mentovai]

Created attachment 42636 [details]
updateCanHaveScrollbars

This is like initScrollbars, but it's called updateCanHaveScrollbars.  It should behave roughly the same way.  The key difference is that it doesn't do the m_needToInitScrollbars thing.

It's hard for me to tell whether that's really necessary, because I can't see who calls -[WebFrameView _install] other than -[WebFrameView _setCustomScrollViewClass:], and I can't see anything that calls that.

If we have to, we can go back to keeping m_needToInitScrollbars, but my chief objection with that was that FrameView was keeping state that was really only used for WebFrameView.

Comment on attachment 42636 [details]
updateCanHaveScrollbars

This version fixes bug 30517 as well. I would r+ it.

[Mark Mentovai]

Comment on attachment 42636 [details]
updateCanHaveScrollbars

I'm much happier with this patch than redoing initScrollbars for the reasons stated above.  Let's go with this, then.

Comment on attachment 42636 [details]
updateCanHaveScrollbars

Thanks for working on this!

I said that I will r+ this patch, but I do have one question and one comment:

>      ScrollbarMode hMode;
>      ScrollbarMode vMode;
> -    scrollbarModes(hMode, vMode);
> +    if (!m_canHaveScrollbars) {
> +        hMode = ScrollbarAlwaysOff;
> +        vMode = ScrollbarAlwaysOff;
> +    } else {
> +        scrollbarModes(hMode, vMode);
> +        if (hMode == ScrollbarAlwaysOff)
> +            hMode = ScrollbarAuto;
> +        if (vMode == ScrollbarAlwaysOff)
> +            vMode = ScrollbarAuto;
> +    }

Can’t you just use m_canHaveScrollbars to choose unconditionally between ScrollbarAlwaysOff and ScrollbarAuto?

> -    void setCanHaveScrollbars(bool flag);
> +    virtual void setCanHaveScrollbars(bool flag);

You should remove the parameter name from the declaration.

As promised, r=me.

[Mark Mentovai]

Created attachment 42665 [details]
Removed parameter name from declaration

Regarding FrameView::layout():
> Can’t you just use m_canHaveScrollbars to choose unconditionally between
> ScrollbarAlwaysOff and ScrollbarAuto?

I don't think so.  The code in here never did that before, it always considered the existing scrollbar state before proceeding.  I think that the non-m_firstLayout case depends on this to do the right thing.

I didn't check now, but I'm pretty sure that I did try to make this change back when I was working on bug 28614, and that it didn't do the right thing.

[Mark Mentovai]

Created attachment 42666 [details]
Make FrameView::layout() start at ScrollbarAuto or ScrollbarAlwaysOff without checking the existing ScrollView state

OK, I tried this and didn't experience any layout test regressions, so I'm taking mitz's other comment about just setting ScrollbarAuto or ScrollbarAlwaysOff and not asking the ScrollView for the existing scrollbar states.

[Mark Mentovai]

I fed the latest patch to the Chromium try bots too, and they also liked it.

[WebKit Commit Bot]

Comment on attachment 42666 [details]
Make FrameView::layout() start at ScrollbarAuto or ScrollbarAlwaysOff without checking the existing ScrollView state

Clearing flags on attachment: 42666

Committed r50665: <http://trac.webkit.org/changeset/50665>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

*** Bug 30517 has been marked as a duplicate of this bug. ***

[Kenneth Rohde Christiansen]

Judging from the buildbot, this patch has broken around 100 Qt layout tests. Can someone investigate?

(In reply to comment #21)
> Judging from the buildbot, this patch has broken around 100 Qt layout tests.
> Can someone investigate?

Please file a new bug.