Closed Bug 1411640 Opened 7 years ago Closed 7 years ago

Consolidate <radio> bindings across platforms

Categories

(Toolkit :: Themes, task, P2)

Product:
Toolkit
Component:
Themes
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: bgrins, Assigned: dao)

References

Details

(Whiteboard: [xbl-flatten-inheritance])

Attachments

(1 file)

Bug 1411640 - Consolidate <radio> bindings and styling across platforms.
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
From https://bugzilla.mozilla.org/show_bug.cgi?id=1410540#c6:
> we should get rid of the platform-specific bindings, which should simplify
> this patch. I did that for <checkbox/> in bug 1345432

For Windows/Linux, we have https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/globalBindings.xml which defines the "radio-with-spacing" and "radio" bindings.
- Linux: <radio> is bound to "radio-with-spacing": https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/global.css#17
- Windows: <radio> is bound to "radio": https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/global.css#17

For OSX we use the standard "radio" binding defined here: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/radio.xml#431.  We also use the standard radio binding cross platform in various places, like subviews in the panelUI: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/customizableui/panelUI.inc.css#1917.

We should at least remove "radio-with-spacing" as in Bug 1345432, but also look into getting rid of the overridden "radio" binding for Windows and make all platforms use the binding defined in radio.xml.
Priority: -- → P2
Blocks: 1412369
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Blocks: 1341048
See Also: → 1347111
Attachment #8924528 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #2)
> Mozscreenshots:
> https://screenshots.mattn.ca/compare/?oldProject=mozilla-
> central&oldRev=40a14ca1cf04499f398e4cb8ba359b39eae4e216&newProject=try&newRev
> =6011efa03c457728a9d9b2caf10d0ee8eebc62f0.

There are some false positives there having to do with the Nightly icon i.e. https://screenshots.mattn.ca/comparisons/mozilla-central/40a14ca1cf04499f398e4cb8ba359b39eae4e216/try/6011efa03c457728a9d9b2caf10d0ee8eebc62f0/linux64/controlCenter_001_noLWT_about.png. Matt, is that something that is expected?  The base is an m-c push and the new is a try push.
Flags: needinfo?(MattN+bmo)
(In reply to Brian Grinstead [:bgrins] from comment #2)
> - And some checkbox spacing changes, which I assume are unintended:
> https://screenshots.mattn.ca/comparisons/mozilla-central/
> 40a14ca1cf04499f398e4cb8ba359b39eae4e216/try/
> 6011efa03c457728a9d9b2caf10d0ee8eebc62f0/windows10-64/
> permissionPrompts_01_noLWT_shareDevices.png.

It's expected ... there was extra space due to this bogus rule:

.checkbox-icon {
  margin-inline-end: 2px;
}
(In reply to Dão Gottwald [::dao] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > - And some checkbox spacing changes, which I assume are unintended:
> > https://screenshots.mattn.ca/comparisons/mozilla-central/
> > 40a14ca1cf04499f398e4cb8ba359b39eae4e216/try/
> > 6011efa03c457728a9d9b2caf10d0ee8eebc62f0/windows10-64/
> > permissionPrompts_01_noLWT_shareDevices.png.
> 
> It's expected ... there was extra space due to this bogus rule:
> 
> .checkbox-icon {
>   margin-inline-end: 2px;
> }

OK sounds fine, I did see that change but wasn't sure why it was there
(In reply to Brian Grinstead [:bgrins] from comment #2)
> - Looks like some slight spacing changes on in content prefs that may not be
> a big deal:
> https://screenshots.mattn.ca/comparisons/mozilla-central/
> 40a14ca1cf04499f398e4cb8ba359b39eae4e216/try/
> 6011efa03c457728a9d9b2caf10d0ee8eebc62f0/linux64/preferences_01_prefsGeneral.
> png 

fixed with the latest patch
OK, I will re-run mozscreenshots on the latest version using a try push for the base so we don't get the logo false positives
New mozscreenshot:

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=6bc0b22a473cae8f91e5d6d5ac9a828362e8351d&newProject=try&newRev=0b6a324628ed8e5206a19a2a568e0a13db75b772

Besides the known windows permission prompt checkbox move I see:

- Very slight radio move on OSX in prefs: https://screenshots.mattn.ca/comparisons/try/6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/0b6a324628ed8e5206a19a2a568e0a13db75b772/osx-10-10/preferences_01_prefsGeneral.png

A couple of things that look like issues with mozscreenshots:

- Text centered on Linux tab on the base rev: https://screenshots.mattn.ca/comparisons/try/6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/0b6a324628ed8e5206a19a2a568e0a13db75b772/linux64/controlCenter_017_noLWT_httpPassword.png
- Panel shadow and background color changes on OSX: https://screenshots.mattn.ca/comparisons/try/6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/0b6a324628ed8e5206a19a2a568e0a13db75b772/osx-10-10/controlCenter_026_darkLWT_https.png
- URL text rendering difference on OSX: https://screenshots.mattn.ca/comparisons/try/6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/0b6a324628ed8e5206a19a2a568e0a13db75b772/osx-10-10/permissionPrompts_33_compactLight_shareVideoAndMicrophone.png
- Border radius difference on windows on about:home https://screenshots.mattn.ca/comparisons/try/6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/0b6a324628ed8e5206a19a2a568e0a13db75b772/windows10-64/primaryUI_001_tabsInTitlebar_fiveTabs_maximized_onlyNavBar_noLWT.png
- Slight desktop background difference (not clipping fully) on Windows: https://screenshots.mattn.ca/comparisons/try/6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/0b6a324628ed8e5206a19a2a568e0a13db75b772/windows10-64/primaryUI_161_tabsOutsideTitlebar_twoPinnedWithOverflow_normal_onlyNavBar_noLWT.png
Whiteboard: [xbl-flatten-inheritance]
(In reply to Brian Grinstead [:bgrins] from comment #9)
> New mozscreenshot:
> 
> https://screenshots.mattn.ca/compare/
> ?oldProject=try&oldRev=6bc0b22a473cae8f91e5d6d5ac9a828362e8351d&newProject=tr
> y&newRev=0b6a324628ed8e5206a19a2a568e0a13db75b772
> 
> Besides the known windows permission prompt checkbox move I see:
> 
> - Very slight radio move on OSX in prefs:
> https://screenshots.mattn.ca/comparisons/try/
> 6bc0b22a473cae8f91e5d6d5ac9a828362e8351d/try/
> 0b6a324628ed8e5206a19a2a568e0a13db75b772/osx-10-10/
> preferences_01_prefsGeneral.png

That's good, it means that Mac was suffering from the bug you reported for Linux in comment 2, and my patch fixes that.
Whiteboard: [xbl-flatten-inheritance]
Whiteboard: [xbl-flatten-inheritance]
Comment on attachment 8924528 [details]
Bug 1411640 - Consolidate <radio> bindings and styling across platforms.

https://reviewboard.mozilla.org/r/195790/#review201212

This is _much_ simpler, thanks! I don't see any visual regressions and the changes look reasonable to me
Attachment #8924528 - Flags: review?(bgrinstead) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e49de4bbc6f
Consolidate <radio> bindings and styling across platforms. r=bgrins
Backed out for failing browser-chrome's browser/base/content/test/static/browser_all_files_referenced.js, at least on Windows:

https://hg.mozilla.org/integration/autoland/rev/4dc4eae45e0a8c6562692a8509b512a8b6a088cb

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4e49de4bbc6f0647dd82b752e52961106e7d4879&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=141936490&repo=autoland

119 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 1, expected 0
120 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: chrome://global/skin/globalBindings.xml -
Flags: needinfo?(dao+bmo)
Huh - I don't see any reference to "toolbarpaletteitem-spacer" other than the definition in globalBindings.xml, so it looks like that file could be removed!

https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/globalBindings.xml?q=toolbarpaletteitem-spacer&redirect_type=single#46
Flags: needinfo?(dao+bmo)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1215f43a62a
Consolidate <radio> bindings and styling across platforms. r=bgrins
https://hg.mozilla.org/mozilla-central/rev/e1215f43a62a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(In reply to Brian Grinstead [:bgrins] from comment #3)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > Mozscreenshots:
> > https://screenshots.mattn.ca/compare/?oldProject=mozilla-
> > central&oldRev=40a14ca1cf04499f398e4cb8ba359b39eae4e216&newProject=try&newRev
> > =6011efa03c457728a9d9b2caf10d0ee8eebc62f0.
> 
> There are some false positives there having to do with the Nightly icon i.e.
> https://screenshots.mattn.ca/comparisons/mozilla-central/
> 40a14ca1cf04499f398e4cb8ba359b39eae4e216/try/
> 6011efa03c457728a9d9b2caf10d0ee8eebc62f0/linux64/
> controlCenter_001_noLWT_about.png. Matt, is that something that is expected?
> The base is an m-c push and the new is a try push.

This is a problem specific to artifact try pushes and I've filed bug 1416538 to address it. In the meantime use the `--no-artifact` option to `mach try fuzzy` if you're going to compare with m-c. I've added a warning to https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots#Try_pushes
Flags: needinfo?(MattN+bmo)
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: