Load toolbarbutton.css and scrollbox.css as document stylesheets

RESOLVED FIXED in Firefox 64

Status

()

defect
P2
normal
RESOLVED FIXED
9 months ago
3 months ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

9 months ago
No description provided.

Comment 2

9 months ago
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+
Assignee

Updated

9 months ago
Depends on: 1340495
Assignee

Comment 3

9 months ago
(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.

Comment 4

9 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d63a6222e9e
Load toolbarbutton.css as a document stylesheet. r=paolo

Comment 5

9 months ago
(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
Assignee

Comment 8

9 months ago
Posted 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)
Assignee

Comment 10

9 months ago
(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)
Assignee

Comment 12

9 months ago
(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.
Assignee

Updated

9 months ago
Summary: Load toolbarbutton.css as a document stylesheet → Load toolbarbutton.css and scrollbox.css as a document stylesheet
Assignee

Updated

9 months ago
Summary: Load toolbarbutton.css and scrollbox.css as a document stylesheet → Load toolbarbutton.css and scrollbox.css as document stylesheets

Updated

9 months ago
Duplicate of this bug: 1473930

Comment 15

9 months ago
Comment on attachment 9010907 [details] [diff] [review]
patch v3

Cool, thanks!
Attachment #9010907 - Flags: review?(paolo.mozmail) → review+

Comment 16

9 months ago
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

Comment 19

9 months ago
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

Comment 20

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff7f29fad49c
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

Updated

9 months ago
Depends on: 1493385

Updated

9 months ago
Depends on: 1493484

Updated

9 months ago
Depends on: 1493555

Updated

9 months ago
Depends on: 1493573

Updated

9 months ago
No longer depends on: 1493484

Updated

9 months ago
Depends on: 1493887
Assignee

Updated

9 months ago
Depends on: 1493638
Assignee

Updated

9 months ago
Depends on: 1493924

Updated

9 months ago
Depends on: 1494204
Assignee

Updated

9 months ago
Blocks: 1489923

Updated

9 months ago
Depends on: 1495442
Assignee

Updated

9 months ago
Depends on: 1495291

Updated

8 months ago
Depends on: 1500647
Assignee

Updated

8 months ago
Depends on: 1501254
Assignee

Updated

3 months ago
No longer depends on: 1493887
Regressions: 1493887
You need to log in before you can comment on or make changes to this bug.