Closed Bug 960277 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox26 --- unaffected
firefox27 + fixed
firefox28 + fixed
firefox29 + fixed

People

(Reporter: roc, Assigned: roc)

References

Details

(Whiteboard: [qa-])

Attachments

(5 files, 1 obsolete file)

This is affecting some Drupal themes, making them unusable in Firefox.
Attached patch backout on betaSplinter Review
[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.
Attachment #8360654 - Flags: approval-mozilla-beta?
The backout is to buy us time to fix this bug for real.
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).
(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?
Flags: needinfo?(roc)
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".
Attachment #8360832 - Flags: review?(matspal) → review+
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.
Attachment #8360867 - Flags: review?(matspal) → review+
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.
Attachment #8360868 - Flags: review?(matspal) → review-
Attached file crashing testcase
Please add this to the test suite.
(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.
Flags: needinfo?(roc)
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.
Yeah, see bug 865602.
Attachment #8360654 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 261037
Blocks: 261037
No longer depends on: 261037
(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.
Attachment #8361592 - Flags: review?(matspal) → review+
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
Attachment #8361592 - Flags: approval-mozilla-aurora?
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.
Attachment #8361592 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
(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.
Keywords: verifyme
Whiteboard: [qa-]
Depends on: 1107508
You need to log in before you can comment on or make changes to this bug.