Closed Bug 1492130 Opened 2 years ago Closed 2 years ago

Load toolbarbutton.css and scrollbox.css as document stylesheets

Categories

(Toolkit :: Themes, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Comment on attachment 9010030 [details] [diff] [review]
patch

I don't see screenshots results for Windows yet, but r=me if they are fine. Thanks!
Attachment #9010030 - Flags: review?(paolo.mozmail) → review+
Depends on: 1340495
(In reply to :Paolo Amadini from comment #2)
> Comment on attachment 9010030 [details] [diff] [review]
> patch
> 
> I don't see screenshots results for Windows yet, but r=me if they are fine.
> Thanks!

The build failed randomly. New push:

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=b0834b9fa59354fc633e5d5ee868ed5c97759e94&newProject=try&newRev=79ad9d62cead0fdec847291fcea2ee573696ae5f&filter=windows

There seems to be a minor difference with .identity-popup-preferences-button's vertical alignment in the control center, but that's not necessarily a bug. Anyway, I can look into that in a followup if needed.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d63a6222e9e
Load toolbarbutton.css as a document stylesheet. r=paolo
(In reply to Dão Gottwald [::dao] from comment #3)
> There seems to be a minor difference with
> .identity-popup-preferences-button's vertical alignment in the control
> center, but that's not necessarily a bug. Anyway, I can look into that in a
> followup if needed.

That small change looks fine to me. Johann?
Flags: needinfo?(jhofmann)
The debug-mochitest (ay11) are also failing on this push: accessible/tests/mochitest/hittest/test_menu.xul
https://treeherder.mozilla.org/logviewer.html#?job_id=200167493&repo=mozilla-inbound&lineNumber=3564
Attached patch patch v2 (obsolete) — Splinter Review
test_bug562554.xul fixed.

> The debug-mochitest (ay11) are also failing on this push:
> accessible/tests/mochitest/hittest/test_menu.xul
> https://treeherder.mozilla.org/logviewer.html#?job_id=200167493&repo=mozilla-
> inbound&lineNumber=3564

Interestingly, that test doesn't even use toolbarbuttons...
Attachment #9010030 - Attachment is obsolete: true
Flags: needinfo?(dao+bmo)
(In reply to :Paolo Amadini from comment #5)
> (In reply to Dão Gottwald [::dao] from comment #3)
> > There seems to be a minor difference with
> > .identity-popup-preferences-button's vertical alignment in the control
> > center, but that's not necessarily a bug. Anyway, I can look into that in a
> > followup if needed.
> 
> That small change looks fine to me. Johann?

Yeah, judging by the screenshot it might be fine, would have to look at it in the context of the other UI items to be sure, but I agree it's not necessarily a bug.
Flags: needinfo?(jhofmann)
(In reply to Dão Gottwald [::dao] from comment #8)
> > The debug-mochitest (ay11) are also failing on this push:
> > accessible/tests/mochitest/hittest/test_menu.xul
> > https://treeherder.mozilla.org/logviewer.html#?job_id=200167493&repo=mozilla-
> > inbound&lineNumber=3564
> 
> Interestingly, that test doesn't even use toolbarbuttons...

Alexander, any idea what's going on with these a11y tests? Why would they be affected by this patch when they don't even have XUL toolbarbutton elements?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29c333cdfb8c39e53e4121b10642bd9990219217
Flags: needinfo?(surkov.alexander)
(In reply to Dão Gottwald [::dao] from comment #10)
> (In reply to Dão Gottwald [::dao] from comment #8)
> > > The debug-mochitest (ay11) are also failing on this push:
> > > accessible/tests/mochitest/hittest/test_menu.xul
> > > https://treeherder.mozilla.org/logviewer.html#?job_id=200167493&repo=mozilla-
> > > inbound&lineNumber=3564
> > 
> > Interestingly, that test doesn't even use toolbarbuttons...
> 
> Alexander, any idea what's going on with these a11y tests? Why would they be
> affected by this patch when they don't even have XUL toolbarbutton elements?
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=29c333cdfb8c39e53e4121b10642bd9990219217

Yeah, a weird one. It seems it fails to open/close a submenu properly (https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/states/test_visibility.xul?q=test_visibility.xul&redirect_type=direct#137). Is there any chance that tooolbarbutton styles are interfered somehow with menu styles? Does anything weird happen visually?
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov (:asurkov) from comment #11)
> (In reply to Dão Gottwald [::dao] from comment #10)
> > (In reply to Dão Gottwald [::dao] from comment #8)
> > > > The debug-mochitest (ay11) are also failing on this push:
> > > > accessible/tests/mochitest/hittest/test_menu.xul
> > > > https://treeherder.mozilla.org/logviewer.html#?job_id=200167493&repo=mozilla-
> > > > inbound&lineNumber=3564
> > > 
> > > Interestingly, that test doesn't even use toolbarbuttons...
> > 
> > Alexander, any idea what's going on with these a11y tests? Why would they be
> > affected by this patch when they don't even have XUL toolbarbutton elements?
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=29c333cdfb8c39e53e4121b10642bd9990219217
> 
> Yeah, a weird one. It seems it fails to open/close a submenu properly
> (https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/
> states/test_visibility.xul?q=test_visibility.xul&redirect_type=direct#137).
> Is there any chance that tooolbarbutton styles are interfered somehow with
> menu styles? Does anything weird happen visually?

Found the problem: the menupopup binding has an anonymous arrowscrollbox, the arrowscrollbox scroll buttons are toolbarbuttons, and they get a different size with this patch applied.
Summary: Load toolbarbutton.css as a document stylesheet → Load toolbarbutton.css and scrollbox.css as a document stylesheet
Summary: Load toolbarbutton.css and scrollbox.css as a document stylesheet → Load toolbarbutton.css and scrollbox.css as document stylesheets
Duplicate of this bug: 1473930
Comment on attachment 9010907 [details] [diff] [review]
patch v3

Cool, thanks!
Attachment #9010907 - Flags: review?(paolo.mozmail) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ced8a51eff4a
Load toolbarbutton.css and scrollbox.css as document stylesheets. r=paolo
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff7f29fad49c
Load toolbarbutton.css and scrollbox.css as document stylesheets. r=paolo
https://hg.mozilla.org/mozilla-central/rev/ff7f29fad49c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1493385
Depends on: 1493390
Depends on: 1493484
Depends on: 1493544
Depends on: 1493555
Depends on: 1493573
No longer depends on: 1493484
Depends on: 1493887
Depends on: 1493638
Depends on: 1493924
Depends on: 1494204
Blocks: 1489923
Depends on: 1495442
Depends on: 1495291
Depends on: 1500647
Depends on: 1501254
No longer depends on: 1493887
Regressions: 1493887
You need to log in before you can comment on or make changes to this bug.