1301290 - Div resize handles are always displayed above textarea resize handles

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Alex Henrie]

Attachment: Demo

This is tricky to explain; please take a look at the attached demo. The problem is that if there is a resizable textarea on top of a resizable div, clicking and dragging the handle of the textarea on top actually clicks and drags the handle of the div on bottom. It's a significant problem for interactive web apps that allow moving and resizing elements.

Chrome does not have any trouble with the attached demo.

Comment 1

[Alex Henrie]

I found a workaround: Adding style="position:relative" to the textarea gets its resize handle on top.

Comment 2

[Alex Henrie]

Mats, this looks similar to bug 631337. Do you have any ideas?

Comment 3

[Mats Palmgren (inactive)]

Yeah, it looks like a similar problem.  This might be a good place to start digging:
http://searchfox.org/mozilla-central/rev/591354c5e5521cf845bf6b6ef44e8b3b0aeda17d/layout/generic/nsGfxScrollFrame.cpp#2991

Comment 4

[Alex Henrie]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27184faf7996

Comment 5

[Alex Henrie]

Attachment: First attempt

For what it's worth, here's my attempt at fixing the bug. It's not a proper solution because it breaks plain textarea resizers (i.e. resizers without scrollbars).

Comment 6

[Alex Henrie]

I did find a better workaround: Adding style="isolation:isolate" to both the div and the textarea fixes the resize handle stacking while still stacking the div and the textarea beneath any positioned elements.

Comment 7

[Alex Henrie]

Turns out "isolation:isolate" isn't a great workaround either because in Chrome isolated elements can be above positioned elements.

Comment 8

[Alex Henrie]

Attachment: [PATCH v2] Position resizer above scroll corner but beneath overlapping elements.

I fixed the problem by special-casing the resizer box to be displayed at the same time as the overlay scrollbars. This seems to be the simplest way to ensure that the resizer box is always above the textbox content, without putting it on top of anything else. It's also not much different from what we were doing before, since kid->IsAbsPosContainingBlock() was only true if kid was mResizerBox.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9479aea7840

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8796884 [details] [diff] [review]
[PATCH v2] Position resizer above scroll corner but beneath overlapping elements.

Looks reasonable, but it breaks the overflow:hidden case in Testcase #2.
Would it be possible to avoid that somehow?

It does fix the testcase in bug 631337 comment 11 though, which is nice.
Could you include that snippet in your reftest please?

Comment 11

[Alex Henrie]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=843201a17262

Comment 12

[Alex Henrie]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d74a3ab7fcd6

Comment 13

[Alex Henrie]

Attachment: [PATCH v3] Position resizer above own content but beneath overlapping content.

This patch revises nsGfxScrollFrame to put the overlay scrollbars and the resizer just barely above the other children. That way, the resizer will always be on top of any other children, but it will stay below the content of other overlapping elements. Textarea resizers are redirected to the content layer, so now that div resizers are typically on the content layer too, they do not automatically jump above textarea resizers.

I decided to add a mochitest instead of reftests because this solution doesn't change the stacking of the resizer in any of the tests we've discussed. I think it would be better to open a new bug to continue discussing in which situations the resizer should be hidden.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b044ee1efeb7

Comment 14

[Mats Palmgren (inactive)]

Do you know if "!outlines->IsEmpty()" is ever true?  If you don't know, can you submit
a Try job with a fatal assertion in that branch and see if any test fails?
If nothing fails I think we should simplify the patch to just:
-      aLists.Outlines()->AppendNewToTop(newItem);
+      aLists.Content()->AppendNewToTop(newItem);

Comment 15

[Alex Henrie]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fa6c744ed3c

Comment 16

[Mats Palmgren (inactive)]

Looking at the failures there it seems we still add to Outlines() in some cases.
I traced a mochitest run of layout/generic/test/test_bug288789.html in rr and
those items are added here:
http://searchfox.org/mozilla-central/rev/c1e745733c84630821ef53754b627f2c0b0b5202/layout/generic/nsGfxScrollFrame.cpp#3584

It's for some "hoisting" functionality I know nothing about.
It looks like those lines originally came from bug 1041510.
http://searchfox.org/mozilla-central/diff/3389fb77b93776a153f5b183f0953802470d5c00/layout/generic/nsGfxScrollFrame.cpp#2649

Timothy, do you remember if that positionedDescendants/Outlines block is just a mirror
of the code in AppendToTop?
http://searchfox.org/mozilla-central/diff/3389fb77b93776a153f5b183f0953802470d5c00/layout/generic/nsGfxScrollFrame.cpp#2125
I have a feeling it might be, and that if we make the suggested change in comment 14
we should probably change this Outlines() to use Content() too?

Comment 17

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #16)
> Looking at the failures there it seems we still add to Outlines() in some
> cases.
> I traced a mochitest run of layout/generic/test/test_bug288789.html in rr and
> those items are added here:
> http://searchfox.org/mozilla-central/rev/
> c1e745733c84630821ef53754b627f2c0b0b5202/layout/generic/nsGfxScrollFrame.
> cpp#3584
> 
> It's for some "hoisting" functionality I know nothing about.
> It looks like those lines originally came from bug 1041510.
> http://searchfox.org/mozilla-central/diff/
> 3389fb77b93776a153f5b183f0953802470d5c00/layout/generic/nsGfxScrollFrame.
> cpp#2649
>
> Timothy, do you remember if that positionedDescendants/Outlines block is
> just a mirror
> of the code in AppendToTop?
> http://searchfox.org/mozilla-central/diff/
> 3389fb77b93776a153f5b183f0953802470d5c00/layout/generic/nsGfxScrollFrame.
> cpp#2125
> I have a feeling it might be, and that if we make the suggested change in
> comment 14
> we should probably change this Outlines() to use Content() too?

I think you're right, they should be kept in sync.

I think the outlines list there can contain any of the outlines for scrolled children, not just for that one item you found.

Comment 18

[Alex Henrie]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4bda8e00340

Comment 19

[Alex Henrie]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd8c015f6a30

Comment 20

[Mats Palmgren (inactive)]

I was thinking something like this might pass tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b5b16aaab892bf494461f12e3230553ad47c877

Comment 21

[Alex Henrie]

Based on these tests, I don't think it's reasonable to expect Outlines() to always be empty. However, putting the overlay scrollbars and the resizer beneath the outlines does not appear to cause problems in practice. What do you want to do?

Comment 22

[Mats Palmgren (inactive)]

I can trigger the assertion in an OSX debug build loading the test
dom/events/test/test_bug226361.xhtml and then tabbing to the <iframe>
there.  That will display a "focus ring" inside the scrolled area.
I'm guessing that's an outline item of some sort on OSX.

I think it's probably fine to skip the Outlines() part then, i.e.
the patch you have in your last push to Try should work.

However, given what @tn said that these blocks should stay in sync,
I think we should try to make a small utility function to encapsulate
that, something like:

template<class T>
static void
AppendInternalItemOnTop(nsDisplayListBuilder* aBuilder,
                        const nsDisplayListSet& aLists,
                        T* aItem,
                        int32_t aZIndex)
{
  nsDisplayList* positionedDescendants = aLists.PositionedDescendants();
  if (aZIndex >= 0 && !positionedDescendants->IsEmpty()) {
    aItem->SetOverrideZIndex(aZIndex);
    positionedDescendants->AppendNewToTop(aItem);
  } else {
    aLists.Content()->AppendNewToTop(aItem);
  }
}

and then call that from both places?

The conditions for when to use positionedDescendants is slightly
different in both places, but I guess
"aZIndex >= 0 && !positionedDescendants->IsEmpty()" works for both.
Alternatively, we could change MaxZIndexInList to return -1 when
the given list is empty and then just test "if (aZIndex >= 0)" above.
That might be a little less convoluted.

What do you think?

Comment 23

[Alex Henrie]

Attachment: [PATCH v4] Position resizer above own content but beneath overlapping content.

Sure, that's fine. I would prefer to make MaxZIndexInList return -1 for an empty list to match MaxZIndexInListOfItemsContainedInFrame.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d07308893f8e

Comment 24

[Mats Palmgren (inactive)]

Comment on attachment 8798323 [details] [diff] [review]
[PATCH v4] Position resizer above own content but beneath overlapping content.

r=mats, with one nit: we should probably name the new function ...ToTop
rather than ...OnTop for consistency with AppendToTop, AppendNewToTop.
(my bad!)

Looks great otherwise, thanks for your perseverance in fixing this bug!

Comment 25

[Alex Henrie]

Attachment: [PATCH v5] Position resizer above own content but beneath overlapping content.

Function renamed, no other changes.

Comment 26

[Alex Henrie]

Requesting checkin of attachment 8798437 [details] [diff] [review], r=mats

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a154874f694
Position resizer above own content but beneath overlapping content. r=mats

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a154874f694

Comment 29

[Alex Henrie]

Thanks Mats, Timothy, and Ryan! I am definitely happy to see this bug die.