631337 - cannot resize Firefox window on XP with Start Page (or any other page with position: relative in the body) loaded

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Asa Dotzler [:asa]]

When I have the sexy new Firefox Start Page loaded up, the resizer widget in the corner of the Window doesn't work. It works when I have other pages loaded. Something in Firefox Start (about:home) is breaking it. 

If this is confirmed, it should block. Testing 2011-02-02 build on Win XP.

Comment 1

[Stephen Horlander [:shorlander]]

The body element on that page has "position: relative". Removing that fixes this issue. Which means any page could cause this problem.

Comment 2

[Marcia Knous [:marcia]]

Using Mozilla/5.0 (Windows NT 5.1; rv:2.0b11) Gecko/20100101 Firefox/4.0b11, I can resize the page with about:home loaded. I am testing in a VM. I will say you have to be pretty precise about whether you place your mouse, but you can resize the page.

Comment 3

[Stephen Horlander [:shorlander]]

(In reply to comment #2)
> Using Mozilla/5.0 (Windows NT 5.1; rv:2.0b11) Gecko/20100101 Firefox/4.0b11, I
> can resize the page with about:home loaded. I am testing in a VM. I will say
> you have to be pretty precise about whether you place your mouse, but you can
> resize the page.

Is this by clicking and dragging on the actual dotted resizer image or on the corner edge of the window frame?

Comment 4

[Asa Dotzler [:asa]]

Marcia is probably seeing what you and I see which is that the dotted triangle is not grabbable but the one pixel or two of Window frame is grabbable.

Comment 5

[Marcia Knous [:marcia]]

yes, I am able to grab the window frame and resize. The dotted triangle doesn't do anything for me on any page.

Comment 6

[Neil Deakin]

Attachment: testcase

Display Lists when resizer is clicked:

Background 0xb033db00(RootBox(window)(-1)) (0,0,66660,42540)(0,0,0,0) uniform
Background 0xb033dbe8(DocElementBox(window)(-1)) (0,0,66660,42540)(0,0,0,0) opaque
Clip (nil)() (0,0,66660,42540)(0,0,0,0)
    Background 0xaed56de0(Deck(deck)(18)) (0,0,66660,42540)(0,0,0,0) uniform
    Background 0xaed56ed8(Box(vbox)(0)) (0,0,66660,42540)(0,0,0,0) uniform
    Background 0xaea24ac8(Box(hbox)(1)) (0,7560,66660,34980)(0,0,0,0) uniform
    Background 0xaea24f10(Box(vbox)(3)) (0,7560,66660,34980)(0,0,0,0) uniform
    Background 0xaea38230(Box(tabbrowser)(0)) (0,7560,66660,34980)(0,0,0,0) uniform
    Background 0xaea38488(Box(tabbox)(-1)) (0,7560,66660,34980)(0,0,0,0) uniform
    Background 0xaea38a40(Deck(tabpanels)(0)) (0,7560,66660,34980)(0,0,0,0) opaque uniform
    Background 0xaea38cc8(XULScroll(notificationbox)(0)) (0,7560,66660,34980)(0,0,0,0) uniform
    Clip (nil)() (0,7560,66660,34980)(0,0,0,0)
        Background 0xaea38c58(Box(notificationbox)(0)) (0,7560,66660,34980)(0,0,0,0) uniform
    Clip (nil)() (0,7560,66660,34980)(0,0,0,0)
        Background 0xaea81130(Stack(stack)(0)) (0,7560,66660,34980)(0,0,0,0) uniform
Clip (nil)() (0,0,66660,42540)(0,0,0,0)
    Clip (nil)() (0,7560,66660,34980)(0,0,0,0)
        WrapList 0xaea81470(FrameOuter(browser)(0)) (0,7560,66660,34980)(0,0,0,0)
            Background 0xaea81470(FrameOuter(browser)(0)) (0,7560,66660,34980)(0,0,0,0) uniform
            Clip 0xaea81470(FrameOuter(browser)(0)) (0,7560,66660,34980)(0,0,0,0)
                OwnLayer 0xae7806d0(Viewport(-1)) (0,7560,66660,34980)(0,0,0,0)
                    Background 0xae780d50(HTMLScroll(html)(-1)) (0,7560,66660,34980)(0,0,0,0) uniform
                    OwnLayer 0xacb0f078(ScrollbarFrame(scrollbar)(-1)) (0,0,0,0)(0,0,0,0)
                    OwnLayer 0xacb2b1f0(ScrollbarFrame(scrollbar)(-1)) (0,0,0,0)(0,0,0,0)
                    OwnLayer 0xad5ea300(Box(scrollcorner)(-1)) (0,0,0,0)(0,0,0,0)
                    Clip (nil)() (0,7560,66660,34980)(0,0,0,0)
                        CanvasBackground 0xae780bd0(Canvas(html)(-1)) (0,7560,66660,34980)(0,0,0,0) uniform
                        Background 0xad5ea598(Block(html)(-1)) (0,7560,66660,34980)(0,0,0,0) uniform
                    OwnLayer 0xad5ea078(Box(resizer)(-1)) (65760,41640,900,900)(0,0,0,0)
                        WrapList 0xad5ea078(Box(resizer)(-1)) (65760,41640,900,900)(0,0,0,0)
                            Background 0xad5ea078(Box(resizer)(-1)) (65760,41640,900,900)(0,0,0,0)
                    Clip 0xad5ea958(Block(body)(1)) (0,7560,66660,34980)(0,0,0,0)
                        WrapList 0xad5ea958(Block(body)(1)) (0,7560,66660,34980)(0,0,0,0)
                            Background 0xad5ea958(Block(body)(1)) (0,7560,66660,34980)(0,0,0,0) uniform

Comment 7

[Neil Deakin]

The hit test returns the last frame.

When position:relative is removed, the last three lines of the display list above are not present.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this looks like a more general graphics or layout issue.
moving to layout and unassigning, doesn't look like something I can figure out with my layout knowledge.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

    ::AppendToTop(aBuilder, aLists.Content(),
                  scrollParts.PositionedDescendants(), mResizerBox,
                  createLayersForScrollbars);

Try appending to aLists.Outlines() instead?

Comment 10

[Neil Deakin]

Attachment: patch

Seems to work

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There is a risk that when resizeable elements are covered by other content, the resizer may be visible even when none of the rest of the element is. But we can't completely avoid that risk and keep the resizer on top of the inner contents at all times. For example, see this testcase:

<div style="resize:both; overflow:hidden; width:100px; height:100px; border:1px solid black">
  <div style="position:relative; top:50px; left:50px; z-index:10; width:100px; height:100px; background:cyan;"></div>
</div>
<div style="background:yellow; position:relative; top:-20px; height:100px; width:100px; background:yellow;"></div>

So I think in general your patch here is what we want. But I think to minimize compatibility risks, we should NOT do this for resizers in textareas, since they have resizers by default and in that case there is no risk of the textarea content being rendered above the resizer, and we don't want textarea resizers to start popping above other content.

The easiest way to do that is probably another flag in nsGfxScrollFrameInner.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We'll need some reftests here too.

Comment 13

[Neil Deakin]

(In reply to comment #9)
>     ::AppendToTop(aBuilder, aLists.Content(),
>                   scrollParts.PositionedDescendants(), mResizerBox,
>                   createLayersForScrollbars);
> 
> Try appending to aLists.Outlines() instead?

Hmm. I'm testing this change again and it doesn't seem to working any more. It's quite possible that I imagined that it was working before.

Any other suggestions?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't understand how that could not work.

Comment 15

[Mats Palmgren (inactive)]

Attachment: make the resizer rel.pos.

The problem is that the rel.pos. <body> is already placed in the
PositionedDescendants() list at that point, so adding the resizer
to Content() or Outlines() isn't going to help.

Maybe we can make the resizer rel.pos. with maximum z-index?
It seems to work (also for the testcase in comment 11).

Comment 16

[Neil Deakin]

Comment on attachment 511073 [details] [diff] [review]
make the resizer rel.pos.

This patch seems to work ok for me.

> resizer {
>   -moz-binding: url("chrome://global/content/bindings/resizer.xml#resizer");
>+  position: relative;
>+  z-index: 2147483647;
> }

Would this change cause any problems for other resizers? Do we want to limit to only the window resizer?

Comment 17

[Mats Palmgren (inactive)]

Attachment: rel.pos. resizer for non-replaced elements with non-empty pos-descendants list

Use the Content() list for replaced elements, and when the
PositionedDescendants() is empty (as nothing can overlap
the resizer in that case).

Comment 18

[Mats Palmgren (inactive)]

(In reply to comment #16)
> Would this change cause any problems for other resizers?

Not sure what kind of problem you're thinking of.

> Do we want to limit to only the window resizer?

It would be easy to do so if we want to minimize the risk.

Comment 19

[Mats Palmgren (inactive)]

Attachment: rel.pos. resizer, but only for the root frame

Comment 20

[Neil Deakin]

(In reply to comment #18)
> Not sure what kind of problem you're thinking of.

Don't know. Does making every resizer relative positioned with a high z-index have any side effects which change the layout in some way?

Comment 21

[Mats Palmgren (inactive)]

Yes, the side-effect that roc noted in comment 11.  If there is surrounding
content that overlaps the resizable element the resizer will show through
[if we place it on the PositionedDescendants() list].
That's why I added the extra bit of heuristics - exclude replaced elements
and when PositionedDescendants() list of the scrolled frame is empty.
It seems worth it to fix the case where a positioned child is below
the resizer.

We could improve it further by testing if the PositionedDescendants() list
has any items that intersect the resizer... is there an easy way to do that?

Comment 22

[Neil Deakin]

I'm not asking about the GfxScrollFrame change, which should only affect painting, no? I'm asking about the change to xul.css and what effect it has.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    PRBool pos =
+      mOuter->GetParent() == mOuter->PresContext()->PresShell()->GetRootFrame();

Use mIsRoot. Otherwise I think the approach in comment #19 is fine.

Comment 24

[Mats Palmgren (inactive)]

(In reply to comment #22)
> I'm not asking about the GfxScrollFrame change, which should only affect
> painting, no?

Right, painting and hit testing for event targeting.

> I'm asking about the change to xul.css and what effect it has.

None, I hope.  I checked the frame trees before and after and I saw no difference
for the resizer.  The computed style is different of course but XUL layout ignores
all positioning AFAIK.  For nsGfxScrollFrame, the resizer is placed explicitly
at its position and I don't see anything that will break by the rel.pos. change.
FWIW, the "attachment 511128 [details] [diff] [review]" patch passed unit tests on TryServer.
I tried a few themes from AMO and they seemed to work fine with the patch. 

There is an existing bug with the resizer though: after resizing the window the
first click after releasing the mouse button is ignored ("sticky effect").
This bug occurs also without my patch so I assume it's a known problem.

Comment 25

[Mats Palmgren (inactive)]

Attachment: rel.pos. resizer, but only for the root frame, v2

This is with roc's suggestion in comment 23.

Comment 26

[Neil Deakin]

(In reply to comment #24)
> There is an existing bug with the resizer though: after resizing the window the
> first click after releasing the mouse button is ignored ("sticky effect").
> This bug occurs also without my patch so I assume it's a known problem.

That might be bug 632953.

Comment 27

[Mats Palmgren (inactive)]

Oh, I just filed bug 633169 because I couldn't find anything that matched.
Yeah, it might be the same underlying problem.

Comment 28

[Mats Palmgren (inactive)]

Comment on attachment 511334 [details] [diff] [review]
rel.pos. resizer, but only for the root frame, v2

I think the other patch is fine too, but this is lower risk.

Comment 29

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/a28d7d3ce0f0

Comment 30

[Alex Lakatos[:AlexLakatos]]

Verified on Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b12) Gecko/20100101 Firefox/4.0b12