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: