Closed Bug 1461793 Opened 6 years ago Closed 6 years ago

Remove the "popup-base" binding and import the "popup.css" file as a document stylesheet

Categories

(Toolkit :: UI Widgets, task, P1)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: bgrins, Assigned: Paolo)

References

(Regressed 1 open bug)

Details

Attachments

(1 file)

Bug 1461793 - Remove the "popup-base" binding and import the "popup.css" file as a document stylesheet.
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
The popup-base binding only loads a stylesheet. It looks like a relatively simple sheet: https://searchfox.org/mozilla-central/search?q=popup.css&redirect=true.

This could be imported into components.css and the binding could be removed: https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/toolkit/content/widgets/popup.xml#14-18.

Doing this should unblock a Custom Element migration of the 'tooltip' binding.
Component: XUL → XUL Widgets
Product: Core → Toolkit
Blocks: 1461798
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8979242 [details]
Bug 1461793 - Remove the "popup-base" binding and import the "popup.css" file as a document stylesheet.

https://reviewboard.mozilla.org/r/245462/#review251446

Code changes look good, thanks. I trust you've looked to make sure we don't hit any XBL->UA glitches.
Attachment #8979242 - Flags: review?(bgrinstead) → review+
I didn't see any "!important" rules, and some quick testing didn't show any regressions.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d511f7fc5b5
Remove the popup-base binding and instead import the popup.css file in components.css. r=bgrins
(In reply to Narcis Beleuzu [:NarcisB] from comment #5)
> toolkit/content/tests/chrome/test_arrowpanel.xul

This fails because the last arrow panel in the test file doesn't animate anymore. Arrow panels in the main user interface, like the main menu, still animate correctly.

Strangely, the styles that define the animation behavior are defined in "xul.css", and we haven't actually changed how that file is loaded here. The inspector isn't of much help, because even when the animation works I don't see any of the styles from "xul.css". Brian, any idea?
Flags: needinfo?(paolo.mozmail) → needinfo?(bgrinstead)
components.css should load after xul.css as per https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/layout/base/nsDocumentViewer.cpp#2493, so rules with the same specificity in components.css should have priority. That said, since components.css loads with a different cascade we could have had a case where a rule with lower specificity used to get applied when it was a XBL sheet but now it doesn't apply anymore. Can you point me to the specific styles that aren't working anymore?

I expect you should be able to see UA styles in the Browser Toolbox without changing any settings. Are you not seeing any UA styles or just not seeing rules from xul.css?
Flags: needinfo?(bgrinstead) → needinfo?(paolo.mozmail)
(In reply to Brian Grinstead [:bgrins] from comment #7)
> Can you point me to the specific styles that aren't working anymore?

These are the styles for Mac, and other platforms are right below:

https://dxr.mozilla.org/mozilla-central/rev/51f2535c797495f1f4e864072c2449b7c28669de/toolkit/content/xul.css#400-431

> I expect you should be able to see UA styles in the Browser Toolbox without
> changing any settings. Are you not seeing any UA styles or just not seeing
> rules from xul.css?

I was using the in-content inspector because the Browser Toolbox (--jsdebugger) wasn't able to inspect the popup, but I was able to fix this issue by closing and reopening its window. The UA styles didn't show in the in-content inspector, which seems expected.

I can now see the UA styles even with the patch applied, and the weird thing is that I don't see anything from the "popup.css" files that could override the animated properties, like "transition" or any -moz-window property...
Flags: needinfo?(paolo.mozmail) → needinfo?(bgrinstead)
By the way, if you do `--setpref devtools.inspector.showAllAnonymousContent=true --setpref devtools.inspector.showUserAgentStyles=true` you can inspect UA / XBL styles from the in-content toolbox.
Full command for local debugging: `./mach mochitest toolkit/content/tests/chrome/test_arrowpanel.xul --setpref devtools.inspector.showAllAnonymousContent=true --setpref devtools.inspector.showUserAgentStyles=true --setpref ui.popup.disable_autohide=true`
Thanks for the tip! Interestingly enough, it seems that scrolling with the wheel in the in-content developer tools is broken when they are opened during the test. I'm not sure if this issue can be related to the other ones with styling.
(In reply to :Paolo Amadini from comment #11)
> Thanks for the tip! Interestingly enough, it seems that scrolling with the
> wheel in the in-content developer tools is broken when they are opened
> during the test. I'm not sure if this issue can be related to the other ones
> with styling.

I've just started noticing that in the last couple of weeks - it's not due to anything in this patch. It only happens when ui.popup.disable_autohide is set.
Flags: needinfo?(bgrinstead)
Just loading an empty stylesheet frokm a "data:" URI in the arrowpanel binding fixes the test failure for me locally.

The reason why this happens is still a mystery, but this is useful as it allows landing the mass migration in bug 1463820.

Brian, do we still want to land this bug individually?
Blocks: 1463820
Flags: needinfo?(bgrinstead)
(In reply to :Paolo Amadini from comment #15)
> Brian, do we still want to land this bug individually?

I'd say let's wait to make a decision in Bug 1463820 before we proceed here.
Flags: needinfo?(bgrinstead)
Blocks: 1470880
Note that the test failure still occurs using "widgets.css", so I filed bug 1470880.
Blocks: 1470830
Summary: Remove the popup-base binding and instead import the popup.css file in components.css → Remove the "popup-base" binding and import the "popup.css" file as a document stylesheet
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e711420b85f7
Remove the "popup-base" binding and import the "popup.css" file as a document stylesheet. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/e711420b85f7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1474538
Backed out for causing bug 1474538:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e41340c3ceef0dc67dda503f3facc1a86cc49614
Status: RESOLVED → REOPENED
status-firefox63: fixed → ---
Resolution: FIXED → ---
Depends on: 1474614
Boris, can you de-classify what the top-secret default tooltip node is about? ;-)

https://searchfox.org/mozilla-central/rev/46292b1212d2d61d7b5a7df184406774727085b8/layout/xul/nsDocElementBoxFrame.cpp#107-118

It is created in nsDocElementBoxFrame::CreateAnonymousContent, which I suspect may be the reason for bug 1474538, which doesn't happen for tooltips with an explicit <tooltip> element defined.

How can we move forward here? Can we maybe create a non-anonymous <tooltip> node, either in the platform or somewhere else in the front-end?
Flags: needinfo?(bzbarsky)
Note that content pages use the "remoteBrowserTooltip", which is a different code path and displays correctly like other <tooltip> elements.
See Also: → 1460691
Assuming the problem is that we need the tooltip styles to be loaded in a UA sheet - the number of styles is really pretty limited: https://searchfox.org/mozilla-central/search?q=%5Etooltip&regexp=true&path=css. So perhaps we could load just those through xul.css, either:
1) as an @import to a new sheet if we need platform-specific styles
2) directly into xul.css if we can somehow consolidate the rules across platforms
I don't know what the default tooltip node is about offhand, sorry...

But yes, it seems plausible that we just need to put the tooltip styles in a sheet that would apply to native anonymous content, to preserve current behavior.
Flags: needinfo?(bzbarsky)
(In reply to :Paolo Amadini from comment #26)
> Boris, can you de-classify what the top-secret default tooltip node is
> about? ;-)
> 
> https://searchfox.org/mozilla-central/rev/
> 46292b1212d2d61d7b5a7df184406774727085b8/layout/xul/nsDocElementBoxFrame.
> cpp#107-118

I don't think this helps explain the problem, but the anonymous tooltip is used as the default when an element uses the "tooltiptext" attribute or if the element needs a tooltip and it's in a XUL tree. Soon it will also be used as the tooltip for top level HTML pages as well (bug 1460691). Search for "GetDefaultTooltip()" if you want to see more about where it's used.
My guess is that Comment 28 option (1) would fix the problem without too much work. So, add a new tooltip.css file gets built for each platform and move the tooltip-related styles from popup.css into it (possibly folding the tooltip[titletip="true"] rule directly into xul.css since it's unchanged across platform). Then @import that file from xul.css.
Attachment #8979242 - Flags: review+ → review?(bgrinstead)
Note that the reference to bug 32157 comment 128 looks unrelated to either the max-width or the commented out margin. The attachment on the bug where the comment was added is currently unavailable due to Bugzilla maintenance, so I can't be sure.
Comment on attachment 8979242 [details]
Bug 1461793 - Remove the "popup-base" binding and import the "popup.css" file as a document stylesheet.

https://reviewboard.mozilla.org/r/245462/#review263896

::: toolkit/content/xul.css:672
(Diff revision 5)
>  }
>  
> +/********** tooltip *********/
> +
> +tooltip[titletip="true"] {
> +  /* The width of the tooltip isn't limited on cropped <tree> cells. */

Should we keep a reference to `bug 32157 comment 128` here? It was pointed to in the copied rule in each popup.css.
Attachment #8979242 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #35)
> Should we keep a reference to `bug 32157 comment 128` here? It was pointed
> to in the copied rule in each popup.css.

Probably not, but see comment 34. We can add it back or translate it to plain English if we find out there is useful information when attachments become available again.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1318ff5a167
Remove the "popup-base" binding and import the "popup.css" file as a document stylesheet. r=bgrins
Backed out changeset c1318ff5a167 (bug 1461793) for automation bustages on Android on a CLOSED TREE

Problematic push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c1318ff5a167e16d3640c1c9c8988b30413091dd&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=188302083
Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=58d36ea57f1c80f7096fa1859da7b5235e3c80b0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=188302083&repo=mozilla-inbound&lineNumber=1103


[task 2018-07-16T07:42:33.916Z] 07:42:33     INFO -  make: *** [build] Error 2
[task 2018-07-16T07:42:33.958Z] 07:42:33     INFO -  249 compiler warnings present.
[task 2018-07-16T07:42:33.996Z] 07:42:33     INFO -  Notification center failed: Install notify-send (usually part of the libnotify package) to get a notification when the build finishes.
[task 2018-07-16T07:42:34.011Z] 07:42:34    ERROR - Return code: 2
[task 2018-07-16T07:42:34.011Z] 07:42:34  WARNING - setting return code to 2
[task 2018-07-16T07:42:34.011Z] 07:42:34    FATAL - 'mach build -v' did not run successfully. Please check log for errors.
[task 2018-07-16T07:42:34.011Z] 07:42:34    FATAL - Running post_fatal callback...
[task 2018-07-16T07:42:34.012Z] 07:42:34    FATAL - Exiting -1
[task 2018-07-16T07:42:34.012Z] 07:42:34     INFO - [mozharness: 2018-07-16 07:42:34.012130Z] Finished build step (failed)
[task 2018-07-16T07:42:34.012Z] 07:42:34     INFO - Running post-run listener: _summarize
[task 2018-07-16T07:42:34.012Z] 07:42:34    ERROR - # TBPL FAILURE #
[task 2018-07-16T07:42:34.012Z] 07:42:34     INFO - [mozharness: 2018-07-16 07:42:34.012429Z] FxDesktopBuild summary:
[task 2018-07-16T07:42:34.012Z] 07:42:34    ERROR - # TBPL FAILURE #
Flags: needinfo?(paolo.mozmail)
(In reply to Brian Grinstead [:bgrins] from comment #35)
> Should we keep a reference to `bug 32157 comment 128` here? It was pointed
> to in the copied rule in each popup.css.

I took a look now and while this is in the same comment block as the commented out margin the two are unrelated, and we don't need to port the reference.
(In reply to :Paolo Amadini from comment #39)
> (In reply to Brian Grinstead [:bgrins] from comment #35)
> > Should we keep a reference to `bug 32157 comment 128` here? It was pointed
> > to in the copied rule in each popup.css.
> 
> I took a look now and while this is in the same comment block as the
> commented out margin the two are unrelated, and we don't need to port the
> reference.

OK, thanks for checking
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e8624a457a6
Remove the "popup-base" binding and import the "popup.css" file as a document stylesheet. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/2e8624a457a6
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Depends on: 1485554
Type: enhancement → task
Regressions: 1629813
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: