Closed Bug 1165973 Opened 9 years ago Closed 9 years ago

Add-on verification report has misaligned content

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: ntim, Assigned: Paenglab)

References

Details

Attachments

(2 files, 5 obsolete files)

See screenshot.
Attached image Screenshot
Component: General → Add-ons Manager
Product: Firefox → Toolkit
Attached patch Bug1165973.patch (obsolete) — Splinter Review
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 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)
Blocks: 1151509
(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?
(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;
}
(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.)
(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.
(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.
Attached patch Bug1165973.patch (obsolete) — Splinter Review
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 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?
Attached patch Bug1165973.patch (obsolete) — Splinter Review
Using outline makes it simpler.
Attachment #8609781 - Attachment is obsolete: true
Attachment #8609781 - Flags: review?(dao)
Attachment #8609847 - Flags: review?(dao)
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)
Attached patch Bug1165973.patch (obsolete) — Splinter Review
(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 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)
(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)
How about switching .text-link in global.css to outline as well? :)
Flags: needinfo?(dao)
Attached patch Bug1165973.patch (obsolete) — Splinter Review
(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 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+
Linux uses .text-link:focus. Should I switch to :-moz-focusring?
Flags: needinfo?(dao)
:focus and :-moz-focusring have the same effect on Linux, but yes, it's better to generally just use :-moz-focusring.
Flags: needinfo?(dao)
Attached patch Bug1165973.patchSplinter Review
Added the Linux part with :-moz-focusring.

Carrying over the r1 from previous patch.
Attachment #8612366 - Attachment is obsolete: true
Attachment #8612772 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/341484e03a4a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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
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 on attachment 8612772 [details] [diff] [review]
Bug1165973.patch

Should be low risk. Taking it.
Attachment #8612772 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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)
(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)
(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.
See Also: → 1243353
You need to log in before you can comment on or make changes to this bug.