Horizontal scroll area does not include right margins

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: neil.kronlage, Assigned: roc)

Tracking

({regression, site-compat})

26 Branch
mozilla31
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox28 affected, firefox29 affected, firefox30 affected, firefox31 affected, firefox-esr24 unaffected)

Details

()

Attachments

(4 attachments, 1 obsolete attachment)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.152 Safari/537.36

Steps to reproduce:

Install FF26 or later on Mac or Windows (FF25 does not show this issue).

Go to http://www.colorizephoto.com/ and move your mouse on to the main photo and then move the mouse so the circle following the mouse goes into the gray area around the photo. 


Actual results:

The image suddenly jumps to the right when you move into the gray area.


Expected results:

The photo should not jump to the right.

Note: the Inspector shows the correct right margins on the image in the center, but the horizontal scrollbar does not allow you to scroll to those margins.  Vertical margins work correctly.
Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9c8ab7e9ae41
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131017025216
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/45d9e6cd3473
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131017030414
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9c8ab7e9ae41&tochange=45d9e6cd3473

Regressed by:
45d9e6cd3473	Robert O'Callahan — Bug 926706. When nsGfxScrollFrameInner::UpdateOverflow decides we need to update the scrollbars, don't reflow the scrollframe with NS_FRAME_IS_DIRTY since that reflows all descendants. Just reflow the frame itself and don't dirty anything else. r=tn
Blocks: 926706
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
Posted file repro.html
Simplified repro demonstrating the issue.  Clicking on the page will reposition a div and cause the scroll region to shrink.
Posted patch fix (obsolete) — Splinter Review
Assignee: nobody → roc
Attachment #8399411 - Flags: review?(dbaron)
Comment on attachment 8399411 [details] [diff] [review]
fix

>     for (nsIFrame* lineFrame = line->mFirstChild;
>          n > 0; lineFrame = lineFrame->GetNextSibling(), --n) {
>       ConsiderChildOverflow(lineAreas, lineFrame);
>+      if (line->IsInline()) {
>+        // The overflow area of inline lines includes the margin-box of the
>+        // frames in the line
>+        lineAreas.UnionAllWith(lineFrame->GetMarginRectRelativeToSelf() +
>+                               lineFrame->GetPosition());
>+      }
>     }

I don't see any code corresponding to this in the normal codepath (nsLineLayout::RelativePositionFrames).  If I'd have found it, I'd just have asked you to put comments between it and this code linking the two together -- except I can't, which makes it seem like this is introducing a new inconsistency between the UpdateOverflow codepath and the reflow codepath rather than fixing one.  I certainly could be missing something, though.

>-  return nsBlockFrameSuper::UpdateOverflow();
>+  nsOverflowAreas overflowAreas;
>+  bool found;
>+  nscoord bottomEdgeOfChildren = reinterpret_cast<nscoord>
>+    (Properties().Get(BottomEdgeOfChildrenProperty(), &found));
>+  if (!found) {
>+    bottomEdgeOfChildren = GetRect().height - GetUsedBorderAndPadding().bottom;
>+  }
>+
>+  ComputeOverflowAreas(nsRect(nsPoint(0, 0), GetSize()), StyleDisplay(),
>+                       bottomEdgeOfChildren, overflowAreas);
>+
>+  return FinishAndStoreOverflow(overflowAreas, GetSize());

This change omits the view-resizing code in nsFrame::UpdateOverflow which seems (I think!) still important.

Otherwise this seems fine, but marking review- because of those 2 issues.
Attachment #8399411 - Flags: review?(dbaron) → review-
Views are created for frames in the following cases:
nsHTMLScrollFrame (nsCSSFrameConstructor::InitializeSelectFrame)
nsSubdocumentFrame (nsSubdocumentFrame::Init)
nsListControlFrame (nsCSSFrameConstructor::ConstructFrameFromItemInternal)
nsObjectFrame (nsCSSFrameConstructor::ConstructFrameFromItemInternal)
nsViewportFrame (nsDocumentViewer::MakeWindow, PresShell::VerifyIncrementalReflow, nsPrintEngine::SetRootView)
nsMenuPopupFrame (nsMenuPopupFrame::CreatePopupView)

So they're never created for block frames anymore. I'll add some assertions documenting this.
I removed the generic margin handling code that you pointed out was wrong, and fixed the bug properly by initializing the line overflow areas with the line bounds.

I also fixed another bug which is that the previous patch ignored overflow contributions from some of the child lists (e.g. abs-pos). This caused failures on try.
Attachment #8399841 - Flags: review?(dbaron)
Comment on attachment 8399837 [details] [diff] [review]
Part 1: Assert that nsViews are only associated with a specific, limited set of frame types

r=dbaron, although I prefer that assertion texts assert the thing you're asserting (which I think makes sense both reading the code and when the assertion fails) rather than having a message that only makes sense when the assertion fails.  Maybe "only specific frame types can have an nsView"?
Attachment #8399837 - Flags: review?(dbaron) → review+
Attachment #8399838 - Flags: review?(dbaron) → review+
Comment on attachment 8399841 [details] [diff] [review]
Part 3: Save a block's 'bottom edge of content' in a frame property if it doesn't match the block's content height, and use it in UpdateOverflow to compute the correct overflow

>-    nsOverflowAreas lineAreas;
>+    nsRect bounds = line->mBounds;
>+    nsOverflowAreas lineAreas(bounds, bounds);

I hope this is equivalent to the nullptr == psd->mFrame case in nsLineLayout::RelativePositionFrames, which I find somewhat opaque, since it's using a bunch of stuff on the PerSpanData for the root span that I'm not in the mood to trace down the origins of.

>+  // Union with child frames, skipping the principal and float lists
>+  // since we already handled those using the line boxes.
>+  nsLayoutUtils::UnionChildOverflow(this, overflowAreas,
>+                                    kPrincipalList | kFloatList);

Given that kSelectPopupList is used on a frame that derives from block, I think it would be good to include the default skip child lists as well.  Maybe even make an enum constant on nsLayoutUtils for them?  Or maybe just change patch 2 so the parameter is *additional* skip child lists, and it |s in the defaults?

r=dbaron with that
Attachment #8399841 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/7c7f680de70e
https://hg.mozilla.org/mozilla-central/rev/82f376050e58
https://hg.mozilla.org/mozilla-central/rev/c6becd0b9a5b
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.