Div resize handles are always displayed above textarea resize handles

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: alexhenrie24, Assigned: alexhenrie24)

Tracking

({testcase})

48 Branch
mozilla52
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Posted file 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.
I found a workaround: Adding style="position:relative" to the textarea gets its resize handle on top.
Mats, this looks similar to bug 631337. Do you have any ideas?
Flags: needinfo?(mats)
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
Flags: needinfo?(mats)
Keywords: testcase
Posted patch First attempt (obsolete) — Splinter Review
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).
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.
Turns out "isolation:isolate" isn't a great workaround either because in Chrome isolated elements can be above positioned 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
Attachment #8796884 - Flags: review?(mats)
Posted file Testcase #2
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?
Flags: needinfo?(alexhenrie24)
Attachment #8796884 - Flags: review?(mats) → review-
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
Assignee: nobody → alexhenrie24
Attachment #8791449 - Attachment is obsolete: true
Attachment #8796884 - Attachment is obsolete: true
Flags: needinfo?(alexhenrie24)
Attachment #8797384 - Flags: review?(mats)
Attachment #8797384 - Flags: feedback?(tnikkel)
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);
Flags: needinfo?(alexhenrie24)
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?
Flags: needinfo?(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.
Flags: needinfo?(tnikkel)
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?
Flags: needinfo?(alexhenrie24) → needinfo?(mats)
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?
Flags: needinfo?(mats) → needinfo?(alexhenrie24)
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
Attachment #8797384 - Attachment is obsolete: true
Attachment #8797384 - Flags: review?(mats)
Attachment #8797384 - Flags: feedback?(tnikkel)
Flags: needinfo?(alexhenrie24)
Attachment #8798323 - Flags: review?(mats)
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!
Attachment #8798323 - Flags: review?(mats) → review+
Function renamed, no other changes.
Attachment #8798323 - Attachment is obsolete: true
Attachment #8798437 - Flags: review+
Requesting checkin of attachment 8798437 [details] [diff] [review], r=mats
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a154874f694
Position resizer above own content but beneath overlapping content. r=mats
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9a154874f694
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Thanks Mats, Timothy, and Ryan! I am definitely happy to see this bug die.
You need to log in before you can comment on or make changes to this bug.