Closed
Bug 1165973
Opened 9 years ago
Closed 9 years ago
Add-on verification report has misaligned content
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla41
People
(Reporter: ntim, Assigned: Paenglab)
References
Details
Attachments
(2 files, 5 obsolete files)
50.61 KB,
image/png
|
Details | |
5.24 KB,
patch
|
Paenglab
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
See screenshot.
Reporter | ||
Comment 1•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Component: General → Add-ons Manager
Product: Firefox → Toolkit
Assignee | ||
Comment 2•9 years ago
|
||
This adds on Windows a padding on the left to align with the other elements. This is only on Windows not aligned. I also added a 48px margin at the right to have the same gap like the lists.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8607699 -
Flags: review?(dtownsend)
Comment 3•9 years ago
|
||
Comment on attachment 8607699 [details] [diff] [review] Bug1165973.patch Review of attachment 8607699 [details] [diff] [review]: ----------------------------------------------------------------- Over to dao
Attachment #8607699 -
Flags: review?(dtownsend) → review?(dao)
Comment 4•9 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #2) > Created attachment 8607699 [details] [diff] [review] > Bug1165973.patch > > This adds on Windows a padding on the left to align with the other elements. > This is only on Windows not aligned. Why is this a problem on Windows but not on Linux or OS X?
Updated•9 years ago
|
status-firefox40:
--- → affected
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #4) > (In reply to Richard Marti (:Paenglab) from comment #2) > > Created attachment 8607699 [details] [diff] [review] > > Bug1165973.patch > > > > This adds on Windows a padding on the left to align with the other elements. > > This is only on Windows not aligned. > > Why is this a problem on Windows but not on Linux or OS X? Because only Windows has a -moz-margin-start: 6px; see https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/global.css#225. I could also set: #disabled-unsigned-addons-heading > description, #disabled-unsigned-addons-heading > label { -moz-margin-start: 0; -moz-margin-end: 0; }
Comment 6•9 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #5) > > Why is this a problem on Windows but not on Linux or OS X? > > Because only Windows has a -moz-margin-start: 6px; see > https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/ > global.css#225. Linux does too. (And OS X should, that's bug 459563.)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #6) > (In reply to Richard Marti (:Paenglab) from comment #5) > > > Why is this a problem on Windows but not on Linux or OS X? > > > > Because only Windows has a -moz-margin-start: 6px; see > > https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/ > > global.css#225. > > Linux does too. (And OS X should, that's bug 459563.) Ah, now at home I see the correct answer is: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/in-content/common.css#75 This is also why I used padding instead of margin to not need an !important.
Comment 8•9 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #7) > Ah, now at home I see the correct answer is: > https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/ > in-content/common.css#75 Let's remove that code then? I don't see why it would be needed on Windows and not on Linux.
Assignee | ||
Comment 9•9 years ago
|
||
Okay, the text-link has now again the normal margins. But this needs some other tweaks to work correctly with the :-moz-focusring. Also the links in Sync pane are now on all platforms correctly aligned.
Attachment #8607699 -
Attachment is obsolete: true
Attachment #8607699 -
Flags: review?(dao)
Attachment #8609781 -
Flags: review?(dao)
Comment 10•9 years ago
|
||
Comment on attachment 8609781 [details] [diff] [review] Bug1165973.patch Ugh. Can we use outline instead of border for those focus rings such that they don't affect layout?
Assignee | ||
Comment 11•9 years ago
|
||
Using outline makes it simpler.
Attachment #8609781 -
Attachment is obsolete: true
Attachment #8609781 -
Flags: review?(dao)
Attachment #8609847 -
Flags: review?(dao)
Comment 12•9 years ago
|
||
Comment on attachment 8609847 [details] [diff] [review] Bug1165973.patch >+/* Don't draw a transparent border for the focusring because when page >+ colors are disabled, the border is drawn in -moz-DialogText */ This comment makes no sense anymore. With your patch we never draw a border. >+ outline: 1px dotted -moz-DialogText; outline: 1px dotted; without -moz-DialogText
Attachment #8609847 -
Flags: review?(dao)
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #12) > Comment on attachment 8609847 [details] [diff] [review] > Bug1165973.patch > > >+/* Don't draw a transparent border for the focusring because when page > >+ colors are disabled, the border is drawn in -moz-DialogText */ > > This comment makes no sense anymore. With your patch we never draw a border. Is /* Never draw a border for the focusring, use outline instead */ better?
Attachment #8609847 -
Attachment is obsolete: true
Attachment #8610234 -
Flags: review?(dao)
Comment 14•9 years ago
|
||
Comment on attachment 8610234 [details] [diff] [review] Bug1165973.patch >+/* Never draw a border for the focusring, use outline instead */ >+xul|*.text-link, >+xul|button > xul|*.button-box, >+xul|menulist > xul|*.menulist-label-box, >+xul|radio > xul|*.radio-label-box, >+xul|checkbox > xul|*.checkbox-label-box { >+ border-width: 0; > } I'd prefer border-style: none. Can you get rid of "xul|button >", "xul|menulist >" etc. in these selectors? >+xul|*.text-link:-moz-focusring, >+xul|*.inline-link:-moz-focusring, >+xul|button:-moz-focusring > xul|*.button-box, >+xul|menulist:-moz-focusring > xul|*.menulist-label-box, >+xul|radio[focused="true"] > xul|*.radio-label-box, >+xul|checkbox:-moz-focusring > xul|*.checkbox-label-box { > border-width: 0; Why would border-width: 0; be needed here?
Attachment #8610234 -
Flags: review?(dao)
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #14) > Comment on attachment 8610234 [details] [diff] [review] > Bug1165973.patch > > >+xul|*.text-link:-moz-focusring, > >+xul|*.inline-link:-moz-focusring, > >+xul|button:-moz-focusring > xul|*.button-box, > >+xul|menulist:-moz-focusring > xul|*.menulist-label-box, > >+xul|radio[focused="true"] > xul|*.radio-label-box, > >+xul|checkbox:-moz-focusring > xul|*.checkbox-label-box { > > border-width: 0; > > Why would border-width: 0; be needed here? Because the rule from https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/global.css#285 adds the border again. I added it because it was simpler than a new rule. Do you prefer I add xul|*.text-link:-moz-focusring { border-style: none; } instead for all?
Flags: needinfo?(dao)
Comment 16•9 years ago
|
||
How about switching .text-link in global.css to outline as well? :)
Flags: needinfo?(dao)
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #14) > Comment on attachment 8610234 [details] [diff] [review] > Bug1165973.patch > > >+/* Never draw a border for the focusring, use outline instead */ > >+xul|*.text-link, > >+xul|button > xul|*.button-box, > >+xul|menulist > xul|*.menulist-label-box, > >+xul|radio > xul|*.radio-label-box, > >+xul|checkbox > xul|*.checkbox-label-box { > >+ border-width: 0; > > } > > I'd prefer border-style: none. Done > Can you get rid of "xul|button >", "xul|menulist >" etc. in these selectors? Done (In reply to Dão Gottwald [:dao] from comment #16) > How about switching .text-link in global.css to outline as well? :) Moved the .text-link rules to global.css
Attachment #8610234 -
Attachment is obsolete: true
Attachment #8612366 -
Flags: review?(dao)
Comment 18•9 years ago
|
||
Comment on attachment 8612366 [details] [diff] [review] Bug1165973.patch >--- a/toolkit/themes/windows/global/global.css >+++ b/toolkit/themes/windows/global/global.css >@@ -269,26 +269,25 @@ label[disabled="true"]:-moz-system-metri > } > > .wizard-box { > padding: 20px 44px 10px; > } > > .text-link { > color: -moz-nativehyperlinktext; >- border: 1px solid transparent; > cursor: pointer; > } > > .text-link:hover { > text-decoration: underline; > } > > .text-link:-moz-focusring { >- border: 1px dotted -moz-DialogText; >+ outline: 1px dotted; > } Please do this for Linux too
Attachment #8612366 -
Flags: review?(dao) → review+
Assignee | ||
Comment 19•9 years ago
|
||
Linux uses .text-link:focus. Should I switch to :-moz-focusring?
Flags: needinfo?(dao)
Comment 20•9 years ago
|
||
:focus and :-moz-focusring have the same effect on Linux, but yes, it's better to generally just use :-moz-focusring.
Flags: needinfo?(dao)
Assignee | ||
Comment 21•9 years ago
|
||
Added the Linux part with :-moz-focusring. Carrying over the r1 from previous patch.
Attachment #8612366 -
Attachment is obsolete: true
Attachment #8612772 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/341484e03a4a
Keywords: checkin-needed
Comment 23•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/341484e03a4a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 24•9 years ago
|
||
I could reproduce on Windows 7 64-bit and Mac OS X 10.9.5. Verified as fixed on those two systems using Nightly 41.0a1 2015-06-01. The issue is not reproducible for my Ubuntu distribution 14.04 LTS 32-bit.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8612772 [details] [diff] [review] Bug1165973.patch Approval Request Comment [User impact if declined]: bad aligned messages and jumping radio buttons in Add-on manager [Describe test coverage new/current, TreeHerder]: [Risks and why]: low, only CSS
Attachment #8612772 -
Flags: approval-mozilla-aurora?
Comment 26•9 years ago
|
||
Comment on attachment 8612772 [details] [diff] [review] Bug1165973.patch Should be low risk. Taking it.
Attachment #8612772 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 28•9 years ago
|
||
The misaligned paragraph issue is still present on Firefox 40.0a2 (2015-06-17) under Mac OS X 10.9.5 - http://i.imgur.com/YzWGKC4.jpg I have noticed that the misaligned paragraph font size is smaller compared with the rest of the text on Mac. On the others platforms, Windows and Ubuntu, the whole text has the same font size. Verified fixed on Firefox 40.0a2 (2015-06-17) under Windows 7 64-bit and Ubuntu 14.04 32-bit. Based on the above mentioned, should I reopen this bug or file a separate one?
Flags: needinfo?(dao)
Comment 29•9 years ago
|
||
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #28) > The misaligned paragraph issue is still present on Firefox 40.0a2 > (2015-06-17) under Mac OS X 10.9.5 - http://i.imgur.com/YzWGKC4.jpg > > I have noticed that the misaligned paragraph font size is smaller compared > with the rest of the text on Mac. On the others platforms, Windows and > Ubuntu, the whole text has the same font size. > > Verified fixed on Firefox 40.0a2 (2015-06-17) under Windows 7 64-bit and > Ubuntu 14.04 32-bit. > > Based on the above mentioned, should I reopen this bug or file a separate > one? Please file a new bug. I suspect the discrepancy is due to bug 459563 being fixed in 41 but not in 40.
Flags: needinfo?(dao)
Comment 30•9 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #29) > Please file a new bug. I suspect the discrepancy is due to bug 459563 being > fixed in 41 but not in 40. Filed Bug 1175872 for the Mac issue. Based on the above mentions, I am marking this bug as Verified Fixed on Firefox 40 since the other issue is tracked separately.
You need to log in
before you can comment on or make changes to this bug.
Description
•