Closed Bug 1168113 Opened 4 years ago Closed 4 years ago

Anonymous custom content container should use absolute position

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox45 fixed, firefox48 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed
firefox48 --- fixed

People

(Reporter: Honza, Assigned: zer0)

References

Details

Attachments

(1 file, 2 obsolete files)

Anonymous custom content container should use position: absolute by default and highlighters (or any other custom anonymous widget) can define position:fixed if needed.

Here is the place where the position is set to fixed:
https://dxr.mozilla.org/mozilla-central/source/layout/style/ua.css#514

This would allow creating anonymous content that can scroll together with the page.

This change would also help to fix PixelPerfect extension
https://github.com/firebug/pixel-perfect/issues/67

Honza
I *think* this should work, but this needs to be tested thoroughly (ideally, with a page that has scrollbars + nested scrolled iframes + ... all sort of crazy stuff):

- ua.css
  div:-moz-native-anonymous.moz-custom-content-container
  -> position: absolute;

- highlighter.css
  :-moz-native-anonymous .highlighter-container
  -> position: fixed;
Assignee: nobody → zer0
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #1)

> - ua.css
>   div:-moz-native-anonymous.moz-custom-content-container
>   -> position: absolute;
> 
> - highlighter.css
>   :-moz-native-anonymous .highlighter-container
>   -> position: fixed;

I ended up to do exactly that; single highlighter can overruled the position by the style attribute when `highlighter-container` is created; so I think it's fine like that.
I tested manually with a page with nested iframe, with scrollbars, with nested html element with scrollbars too; do I have to write some test as well? But I'm not sure what I could automate here.
Flags: needinfo?(pbrosset)
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #2)
> I ended up to do exactly that; single highlighter can overruled the position
> by the style attribute when `highlighter-container` is created; so I think
> it's fine like that.
> I tested manually with a page with nested iframe, with scrollbars, with
> nested html element with scrollbars too; do I have to write some test as
> well? But I'm not sure what I could automate here.
I think we already do have some tests that check that the highlighter is positioned correctly in /browser/devtools/inspector/test (or wherever this maps to in the new folder structure now).
If current tests pass, that's a good start.
Then, you could take a look at the tests, see if they offer enough of a test coverage for what you're changing it.
Flags: needinfo?(pbrosset)
Attachment #8679857 - Flags: review?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #3)

> > I tested manually with a page with nested iframe, with scrollbars, with
> > nested html element with scrollbars too; do I have to write some test as
> > well? But I'm not sure what I could automate here.

> I think we already do have some tests that check that the highlighter is
> positioned correctly in /browser/devtools/inspector/test (or wherever this
> maps to in the new folder structure now).
> If current tests pass, that's a good start.

As mentioned before, unfortunately our tests pass anyway.
I tried to fix them and add new tests, but the main issue here is that we do not have access to the anonymous content nodes itself.
Therefore all our tests relies on some assumptions: for example, to test the position of the box model, we check the "d" attribute of the svg path. And those values are relative, not absolute. We do not have a way to access to the quads of the element's highlighter, in short. So if the parent's element of our highlighter change its position, our tests do not have a way to know it because the values in the highlighter's node are always the same; and we don't have a way to get the quads of the highlighter, 'cause we do not have access to the node.

We could check if it's possible add a new method in the Anonymous Content API to do so, but we could have some issue with the reflow, and I think it's definitely out of the scope of this bug – we shouldn't block the landing of this bug for something like that, IMVHO.
Comment on attachment 8679857 [details] [diff] [review]
Anonymous custom content container should use absolute position

Review of attachment 8679857 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ua.css
@@ +504,4 @@
>    visibility: hidden;
>  }
>  
>  /* Custom content container in the CanvasFrame, fixed positioned on top of

Please change the comment too (it mentions the fixed positioning).
Attachment #8679857 - Flags: review?(pbrosset) → review+
Attachment #8679857 - Attachment is obsolete: true
Attachment #8688461 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a76322320837
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
This isn't FIXED. The last patch that went in in comment 11 is a CLOBBER change only. The actual fix for this bug has been backed out (comment 9).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/5732e276c41c
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Reopening per comment 13.

The reason this broke things is because it's violating layout invariants.  Do NOT change ua.css without getting review from someone who knows something about the rules in there, please.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Boris Zbarsky [:bz] from comment #15)
> Reopening per comment 13.
> 
> The reason this broke things is because it's violating layout invariants. 
> Do NOT change ua.css without getting review from someone who knows something
> about the rules in there, please.
Really sorry about that, and thanks for reminding me about this. I completely forgot to delegate the review to someone else when I saw the changes in ua.css. I will make sure to keep that in mind.
(In reply to Boris Zbarsky [:bz] from comment #15)

> The reason this broke things is because it's violating layout invariants. 
> Do NOT change ua.css without getting review from someone who knows something
> about the rules in there, please.

Sorry for the mess, Boris! We clearly needs help to fix this, do you have someone in mind or could you give an hand? We're starting to reach the point where highlighters in devtools needs badly this patch.
Flags: needinfo?(bzbarsky)
You basically need bug 1236828 fixed, yes?  If so, I suggest talking to Xidorn.
Flags: needinfo?(bzbarsky)
AFAIK, the container now has "-moz-top-layer: top", so the anonymous contents should be on their own layer. If APZ is enabled, they won't scroll together with the page as the comment 0 needed even if the container is using "position: absolute". Any ideas, Xidorn?

[1] https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/layout/style/res/ua.css#436
Blocks: 1249201
Flags: needinfo?(quanxunzhen)
As :bz said, we just need bug 1236828. That bug doesn't have a high priority currently because I haven't started implementing <dialog>. But if that is necessary for this bug, and this bug has a relatively higher priority, I can do it soon.
Flags: needinfo?(quanxunzhen)
Xidorn, I think this bug is starting to have an higher priority since is a blocker for few other bugs.

It's true than we can workaround with a more complex implementation from JS side, but as Ting-Yu pointed out, now with APZ enabled such implementation is not visually good anymore. So fixing bug 1236828 will save us from unnecessary code to keep things aligned with the page, and will fix the lagging caused by APZ.
Depends on: 893583, 1236828
No longer depends on: 893583
After Bug 1236828 is fixed, is there anything blocking the patch attached in comment 7 from being landed?
Flags: needinfo?(zer0)
Attachment #8688461 - Attachment is obsolete: true
Comment on attachment 8742297 [details] [diff] [review]
Anonymous custom content container should use absolute position. (v2)

Xidorn, does this look ok?
Attachment #8742297 - Flags: review?(bzbarsky) → review?(bugzilla)
Comment on attachment 8742297 [details] [diff] [review]
Anonymous custom content container should use absolute position. (v2)

Review of attachment 8742297 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I think it should work in theory. If it doesn't, there would likely be some bug in our top layer impl which should be fixed separately.
Attachment #8742297 - Flags: review?(bugzilla) → review+
More try on OS X, Windows, and Android. All look green.  If devtools requires no more changes to the patch, let's land this.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fb01fa2fa92&selectedJob=19650364
That's should be enough, thanks! Let's land this.
Flags: needinfo?(zer0)
https://hg.mozilla.org/mozilla-central/rev/a1a6a81a37b7
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.