960277 - absolutely positioned content in <legend> does not receive pointer events when its <fieldset> is an abs-pos container

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

This is affecting some Drupal themes, making them unusable in Firefox.

Comment 1

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

Attachment: backout on beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 261037
User impact if declined: Some Drupal sites unusable in Firefox
Testing completed (on m-c, etc.): none
Risk to taking this patch (and alternatives if risky): Low risk. This is a straightforward backout. The original bug was (and is) nice to have, but was not actively hurting our users.
String or IDL/UUID changes made by this patch: None.

Comment 2

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

The backout is to buy us time to fix this bug for real.

Comment 3

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

As for fixing this on central: the bug is relatively straightforward. The frame tree looks like this:

FieldSet(fieldset)(0)@0x7fffd62f4068 {3000,0,57840,7296}
  Legend(legend)(1)@0x7fffd62f4bd8 next=0x7fffd62f4588 {720,60,240,0} [state=0000168000d00200]
    line 0x7fffd79ca308:
      Placeholder(div)(1)@0x7fffd79ca150 {120,0,0,0} outOfFlowFrame=Block(div)(1)@0x7fffd79ca0b8
    >
  >
  Block(fieldset)(0)@0x7fffd62f4588 {120,120,57600,7056}
    AbsoluteList 0x7fffd479fee0 <
      Block(div)(1)@0x7fffd79ca0b8 {0,0,4860,1140}
        line 0x7fffd79ca2c8:
          Text(0)"HelloKitty"@0x7fffd79ca258 {0,0,4860,1140}

We construct a display list that looks like this:

      WrapList 0x7fffda9cd068(FieldSet(fieldset)(0)) bounds(3480,9120,57840,7296)
        FieldSetBorderBackground 0x7fffda9cd068(FieldSet(fieldset)(0)) bounds(3480,9120,57840,7296)
        BackgroundColor 0x7fffda9cdbd8(Legend(legend)(1)) bounds(4200,9180,240,0)
      WrapList 0x7fffdb5270b8(Block(div)(1)) bounds(3540,9180,4980,1260)
        BackgroundColor 0x7fffdb5270b8(Block(div)(1)) bounds(3600,9240,4860,1140)
        Text 0x7fffdb527258(Text(0)"HelloKitty") bounds(3540,9180,4980,1260)
      BackgroundColor 0x7fffda9cd588(Block(fieldset)(0)) bounds(3600,9240,57600,7056)

That last BackgroundColor is the problem. It's the background of the anonymous inner child, being generated for event purposes. It shouldn't be on top of the <div>, because the <div> is positioned; the problem is we think the anonymous inner child is positioned too, because it's inheriting 'position' from the fieldset (in order to behave as an abs-pos container, basically).

Comment 4

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

Attachment: Part 1: Don't optimize away frame reconstruction for 'transform' add/remove if the element has complex abs-pos container structure

This fixes an existing bug that I found which would also affect fieldsets.

Comment 5

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

Attachment: Part 2: Move nsFieldSetFrame declaration to nsFieldSetFrame.h

Comment 6

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

Attachment: Part 3: nsFieldSetFrame's anonymous child should not inherit CSS 'position', but should still be an abs-pos containing block if the fieldset is

Comment 7

[bhavana bajaj [:bajaj]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away January 20-24) from comment #2)
> The backout is to buy us time to fix this bug for real.

roc, Is this a full backout of 261037 on Fx27? Also just to be sure, the plan on aurora is to leave it in and have a fix in this bug?

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8360832 [details] [diff] [review]
Part 1: Don't optimize away frame reconstruction for 'transform' add/remove if the element has complex abs-pos container structure

r=mats, nit: add a period after "correctly".

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8360867 [details] [diff] [review]
Part 2: Move nsFieldSetFrame declaration to nsFieldSetFrame.h

r=mats

It's fine as is, but if you feel so inclined, then I'd prefer
if you moved back the function bodies of the virtual methods to
the .cpp file.  Perhaps you can then change the #include
nsGkAtoms.h to a forward declaration?  And, while we're here,
please move the '{' after "public nsContainerFrame" to the next
line.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8360868 [details] [diff] [review]
Part 3: nsFieldSetFrame's anonymous child should not inherit CSS 'position', but should still be an abs-pos containing block if the fieldset is

Can 'absPosCBCandidate' really be null on line 5665?
Both the if-statements above it has "absPosCBCandidate->GetType()"
in their last line, and if we take neither of those we're
back to "absPosCBCandidate = frame" and we know 'frame' is non-null.
Am I missing something?

> nsIScrollableFrame* scrollFrame = do_QueryFrame(frame);
> absPosCBCandidate = scrollFrame->GetScrolledFrame();

s/frame/absPosCBCandidate/ or it will crash for scrollable
fieldsets.

Comment 11

[Mats Palmgren (inactive)]

Attachment: crashing testcase

Please add this to the test suite.

Comment 12

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

(In reply to bhavana bajaj [:bajaj] from comment #7)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away January
> 20-24) from comment #2)
> > The backout is to buy us time to fix this bug for real.
> 
> roc, Is this a full backout of 261037 on Fx27?

Yes.

> Also just to be sure, the plan on aurora is to leave it in and have a fix in this bug?

Currently, although it's possible we might end up with a full backout everywhere depending on how spec discussions go.

Comment 13

[Mats Palmgren (inactive)]

I have a vague memory of nsFieldSetFrame::GetInner() returning null in some
cases (overflow containers maybe?), so please add a null check there,
unless you think it becomes a non-issue with these changes.

Comment 14

[Mats Palmgren (inactive)]

Yeah, see bug 865602.

Comment 15

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-beta/rev/00b274fbebf3

Comment 16

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

Attachment: Part 3 v2

Comment 17

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

(In reply to Mats Palmgren (:mats) from comment #9)
> It's fine as is, but if you feel so inclined, then I'd prefer
> if you moved back the function bodies of the virtual methods to
> the .cpp file.  Perhaps you can then change the #include
> nsGkAtoms.h to a forward declaration?  And, while we're here,
> please move the '{' after "public nsContainerFrame" to the next
> line.

Done.

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/997c9994cf6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb7841990d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce27f9e743f

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/997c9994cf6b
https://hg.mozilla.org/mozilla-central/rev/0cb7841990d8
https://hg.mozilla.org/mozilla-central/rev/6ce27f9e743f

Comment 20

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

Comment on attachment 8361592 [details] [diff] [review]
Part 3 v2

Requesting Aurora approval for all 3 patchs.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 261037
User impact if declined: Many Drupal admin pages unusable
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): moderate risk. the change is not trivial. The alternative is to back out 261037 on Aurora like we did for Beta.
String or IDL/UUID changes made by this patch: none

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8361592 [details] [diff] [review]
Part 3 v2

Let's get this in on Aurora and see how it goes on the first beta or two - if we have to, we can backout again before 28 ships.

Comment 22

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac77f02b3dbe
https://hg.mozilla.org/releases/mozilla-aurora/rev/f51c653bd79a
https://hg.mozilla.org/releases/mozilla-aurora/rev/7832d9a8974e

Comment 23

[Andrei Vaida [:avaida]]

Hi Robert, I read about pointer-events (http://mzl.la/LLuPph) and also about the target property of event objects (http://mzl.la/1ao1BI7), but from what I was able to see and please correct me if I'm wrong, attachment 8360958 [details] doesn't make use of the <legend> tag. Could you please confirm if attachment 8360958 [details] is a reliable way of verifying the fix for this bug?

Comment 24

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

That's just a testcase that started crashing due to an earlier version of the patch. The checked-in patch contains an automated testcase so I don't think you need to verify this bug.

Comment 25

[Andrei Vaida [:avaida]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> That's just a testcase that started crashing due to an earlier version of
> the patch. The checked-in patch contains an automated testcase so I don't
> think you need to verify this bug.

Thank you for the clarification. Tagging this as [qa-] based on Comment 24.