Bug 29167 - REGRESSION (r48064): mint.com loses scrollbars after coming out of edit mode
Summary: REGRESSION (r48064): mint.com loses scrollbars after coming out of edit mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://www.mint.com
Keywords: InRadar, Regression
: 30517 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-09-10 20:40 PDT by Alice Liu
Modified: 2010-04-04 20:11 PDT (History)
6 users (show)

See Also:


Attachments
Possible approach (not for review or checkin) (8.05 KB, patch)
2009-09-10 21:38 PDT, Mark Mentovai
no flags Details | Formatted Diff | Diff
Track "can have scrollbar" state within FrameView independently of the individual scrollbar states in ScrollView (7.91 KB, patch)
2009-11-05 21:32 PST, Mark Mentovai
mitz: review-
Details | Formatted Diff | Diff
Track "can have scrollbar" state within FrameView independently of the individual scrollbar states in ScrollView (7.65 KB, patch)
2009-11-05 23:03 PST, Mark Mentovai
no flags Details | Formatted Diff | Diff
With extra logic in resetScrollbars (8.11 KB, patch)
2009-11-05 23:07 PST, Mark Mentovai
no flags Details | Formatted Diff | Diff
updateCanHaveScrollbars (12.21 KB, patch)
2009-11-05 23:42 PST, Mark Mentovai
no flags Details | Formatted Diff | Diff
Removed parameter name from declaration (12.15 KB, patch)
2009-11-06 11:56 PST, Mark Mentovai
no flags Details | Formatted Diff | Diff
Make FrameView::layout() start at ScrollbarAuto or ScrollbarAlwaysOff without checking the existing ScrollView state (12.02 KB, patch)
2009-11-06 12:12 PST, Mark Mentovai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alice Liu 2009-09-10 20:40:34 PDT
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.
Comment 1 Mark Rowe (bdash) 2009-09-10 21:03:12 PDT
<rdar://problem/7215132>
Comment 2 Mark Mentovai 2009-09-10 21:29:52 PDT
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.
Comment 3 Mark Mentovai 2009-09-10 21:38:56 PDT
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 4 mitz 2009-11-05 12:15:13 PST
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.
Comment 5 Mark Mentovai 2009-11-05 12:19:44 PST
OK, I should be able to get to this today or tomorrow.
Comment 6 Mark Mentovai 2009-11-05 21:32:02 PST
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 7 mitz 2009-11-05 21:54:27 PST
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.
Comment 8 Mark Mentovai 2009-11-05 23:03:17 PST
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).
Comment 9 Mark Mentovai 2009-11-05 23:07:03 PST
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.
Comment 10 mitz 2009-11-05 23:31:05 PST
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.
Comment 11 Mark Mentovai 2009-11-05 23:42:00 PST
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 12 mitz 2009-11-06 10:46:26 PST
Comment on attachment 42636 [details]
updateCanHaveScrollbars

This version fixes bug 30517 as well. I would r+ it.
Comment 13 Mark Mentovai 2009-11-06 11:12:56 PST
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 14 mitz 2009-11-06 11:22:25 PST
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.
Comment 15 Mark Mentovai 2009-11-06 11:56:41 PST
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.
Comment 16 Mark Mentovai 2009-11-06 12:12:19 PST
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.
Comment 17 Mark Mentovai 2009-11-06 14:37:35 PST
I fed the latest patch to the Chromium try bots too, and they also liked it.
Comment 18 WebKit Commit Bot 2009-11-09 09:54:40 PST
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>
Comment 19 WebKit Commit Bot 2009-11-09 09:54:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 mitz 2009-11-09 09:56:28 PST
*** Bug 30517 has been marked as a duplicate of this bug. ***
Comment 21 Kenneth Rohde Christiansen 2009-11-09 17:46:50 PST
Judging from the buildbot, this patch has broken around 100 Qt layout tests. Can someone investigate?
Comment 22 mitz 2009-11-09 17:50:12 PST
(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.