The default bug view has changed. See this FAQ.

Add-on verification report has misaligned content

VERIFIED FIXED in Firefox 40

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: ntim, Assigned: Paenglab)

Tracking

Trunk
mozilla41
Points:
---

Firefox Tracking Flags

(firefox40 verified, firefox41 verified)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

2 years ago
See screenshot.
(Reporter)

Comment 1

2 years ago
Created attachment 8607117 [details]
Screenshot
(Reporter)

Updated

2 years ago
Component: General → Add-ons Manager
Product: Firefox → Toolkit
(Assignee)

Comment 2

2 years ago
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.

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)

Updated

2 years ago
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?

Updated

2 years ago
status-firefox40: --- → affected
(Assignee)

Comment 5

2 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;
}
(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

2 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.
(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

2 years ago
Created attachment 8609781 [details] [diff] [review]
Bug1165973.patch

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?
(Assignee)

Comment 11

2 years ago
Created attachment 8609847 [details] [diff] [review]
Bug1165973.patch

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)
(Assignee)

Comment 13

2 years ago
Created attachment 8610234 [details] [diff] [review]
Bug1165973.patch

(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)
(Assignee)

Comment 15

2 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)
How about switching .text-link in global.css to outline as well? :)
Flags: needinfo?(dao)
(Assignee)

Comment 17

2 years ago
Created attachment 8612366 [details] [diff] [review]
Bug1165973.patch

(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+
(Assignee)

Comment 19

2 years ago
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)
(Assignee)

Comment 21

2 years ago
Created attachment 8612772 [details] [diff] [review]
Bug1165973.patch

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

2 years ago
Keywords: checkin-needed

Comment 22

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/341484e03a4a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/341484e03a4a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: affected → fixed
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
status-firefox41: fixed → verified
(Assignee)

Comment 25

2 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 on attachment 8612772 [details] [diff] [review]
Bug1165973.patch

Should be low risk. Taking it.
Attachment #8612772 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b407b0f36675
status-firefox40: affected → fixed
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.
status-firefox40: fixed → verified

Updated

a year ago
Duplicate of this bug: 1136645

Updated

a year ago
See Also: → bug 1243353
You need to log in before you can comment on or make changes to this bug.