Closed Bug 1018703 Opened 8 years ago Closed 8 years ago

in-content preferences: background margin issue when hovering on dropdown items with separators

Categories

(Firefox :: Preferences, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 37

People

(Reporter: xtc4uall, Assigned: shubham, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=css])

Attachments

(6 files, 4 obsolete files)

The background does not "reach" up to the separator (from both sides) when hovering over the dropdown's items.

Can be seen in the (legacy) Sync "Manage Account" dropdown and in the Application's "Action" dropdown.
In [0], you need to remove the top and bottom margin.


[0] : http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#323
Mentor: jaws, ntim007
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [good first bug][lang=css]
Also, while we're there, the separator border-color should be changed to #c1c1c1 to match the latest specs.
Hi Tim,
I don't have source directory inside mozilla-central. Did I miss something while building the source code?
(In reply to Tim Nguyen [:ntim] from comment #1)
> In [0], you need to remove the top and bottom margin.
> [0] :
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-
> content/common.inc.css#323

I tried using the Inspector Tool. Setting the margin-top:0px and margin-bottom:-2px makes the background reach up to the separator.
(In reply to Shubham Jindal from comment #3)
> Hi Tim,
> I don't have source directory inside mozilla-central. Did I miss something
> while building the source code?

You need to go directly to the toolkit directory, ignore the source directory, that's an addition of mxr.
(In reply to Shubham Jindal from comment #4)
> (In reply to Tim Nguyen [:ntim] from comment #1)
> > In [0], you need to remove the top and bottom margin.
> > [0] :
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-
> > content/common.inc.css#323
> 
> I tried using the Inspector Tool. Setting the margin-top:0px and
> margin-bottom:-2px makes the background reach up to the separator.

Actually, you can just remove the margin from menuseparator. That works for me using the inspector at least.
Attached image bug1018703
I have removed the margins as you can see from the Console. But the background has not yet filled up till separator.
(In reply to Shubham Jindal from comment #7)
> Created attachment 8532894 [details]
> bug1018703
> 
> I have removed the margins as you can see from the Console. But the
> background has not yet filled up till separator.

Weird, it works fine on Windows. Can you try setting margin: 0; on the menuitems ?
Attached image Issue with margins
Setting margin:0px on menuseparator kind of solved the problem. There is just a small gap between the separator and "Use Adobe Reader(Default)" which I think is we can work with.
There is no gap between Save File and separator though..:)
Setting margin:0 on menuitem didn't change anything.
(In reply to Shubham Jindal from comment #9)
> Created attachment 8532909 [details]
> Issue with margins
> 
> Setting margin:0px on menuseparator kind of solved the problem. There is
> just a small gap between the separator and "Use Adobe Reader(Default)" which
> I think is we can work with.
> There is no gap between Save File and separator though..:)
> Setting margin:0 on menuitem didn't change anything.

I would recommend to enable the option "Show browser styles" in the devtools settings, so you can clearly see which UA rules are being applied. I would also recommend to check the box-model tab so you can see if there is a margin applied. :) I don't have OSX, so I can't really help you here :/
Attached image Problem Solved
The box-model really helped and browser styles really helped. There was a padding and margins on top and bottom on menu-separator by browser styling.
Adding margin: 0px !important; padding-top: 0px !important; padding-bottom: 0px !important solved the problem. :)
(In reply to Shubham Jindal from comment #11)
> Created attachment 8532913 [details]
> Problem Solved
> 
> The box-model really helped and browser styles really helped. There was a
> padding and margins on top and bottom on menu-separator by browser styling.
> Adding margin: 0px !important; padding-top: 0px !important; padding-bottom:
> 0px !important solved the problem. :)

Awesome :) Are you sure the !important's are necessary though ?
Yes. Just figured out. !importants' are necessary for padding-top and padding-bottom otherwise the browser styles which have !important in it's padding styling overrides the styling of menu-separator.
Comment on attachment 8532926 [details] [diff] [review]
Soved the issue of in-content preferences: background margin issue when hovering on dropdown items with separators

Review of attachment 8532926 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch :) 

While you're here, can you also change the border-color in [0] to #c1c1c1 ? (we want the menupopup border to match the separator border as well).
One other thing, can you change the commit message to something like this : "Bug 1018703 - In-content prefs : Remove spacing around menupopup separators. r=jaws" ?

[0] : http://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#289

::: toolkit/themes/shared/in-content/common.inc.css
@@ +323,5 @@
>  xul|menulist > xul|menupopup xul|menuseparator,
>  xul|button[type="menu"] > xul|menupopup xul|menuseparator {
>    -moz-appearance: none;
> +  padding-top: 0px !important;
> +  padding-bottom: 0px !important;

Why not just padding: 0 !important; ?

@@ +324,5 @@
>  xul|button[type="menu"] > xul|menupopup xul|menuseparator {
>    -moz-appearance: none;
> +  padding-top: 0px !important;
> +  padding-bottom: 0px !important;
> +  margin: 0px;

nit : Please remove the units
Attachment #8532926 - Flags: review?(ntim007) → feedback+
Yeah. I should just go with padding: 0 !important. I was actually begin specific here.:)
Should I add jaws as the reviewer in my new patch?
(In reply to Shubham Jindal from comment #16)
> Yeah. I should just go with padding: 0 !important. I was actually begin
> specific here.:)
> Should I add jaws as the reviewer in my new patch?

Yes :) I'm not a peer, I don't have the permission to review patches.
Attached patch bug1018703.diff (obsolete) — Splinter Review
Attachment #8532926 - Attachment is obsolete: true
Attachment #8532931 - Flags: review?(jaws)
Attachment #8532931 - Flags: feedback?(ntim007)
Comment on attachment 8532931 [details] [diff] [review]
bug1018703.diff

Review of attachment 8532931 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/themes/shared/in-content/common.inc.css
@@ +323,5 @@
>  xul|menulist > xul|menupopup xul|menuseparator,
>  xul|button[type="menu"] > xul|menupopup xul|menuseparator {
>    -moz-appearance: none;
> +  padding: 0 !important;
> +  margin: 0;

nit : margin should be before the padding
Attachment #8532931 - Flags: feedback?(ntim007) → feedback+
Attached patch bug1018703.diff (obsolete) — Splinter Review
Attachment #8532931 - Attachment is obsolete: true
Attachment #8532931 - Flags: review?(jaws)
Attachment #8532936 - Flags: review?(jaws)
Attachment #8532936 - Flags: feedback?(ntim007)
Attachment #8532936 - Flags: feedback?(ntim007) → feedback+
Assignee: nobody → shubhamjindal18
Status: NEW → ASSIGNED
Just out of curiosity, why did we actually place margin above padding? And can you suggest me a new bug..:)
(In reply to Shubham Jindal from comment #21)
> Just out of curiosity, why did we actually place margin above padding?
I don't really know :) If you do the opposite, people won't bug you about that, but we'd like to avoid as much unnecessary changes as we can.

> And can you suggest me a new bug..:)
Bug 1098420 is the easy one left. :)
Or if you'd like more challenging bugs : bug 1089240, bug 1108288 or bug 994555
Comment on attachment 8532936 [details] [diff] [review]
bug1018703.diff

Review of attachment 8532936 [details] [diff] [review]:
-----------------------------------------------------------------

r+ from me if you remove the `!important` and you test that it still works as desired. If the removal reintroduces an issue, please flag me for needinfo and include a reference to the rule that is specifying `!important`.

::: toolkit/themes/shared/in-content/common.inc.css
@@ +323,5 @@
>  xul|menulist > xul|menupopup xul|menuseparator,
>  xul|button[type="menu"] > xul|menupopup xul|menuseparator {
>    -moz-appearance: none;
> +  margin: 0;
> +  padding: 0 !important;

It works for me without the `!important`. The other rule that is specifying padding is from menu.css for "menulist > menupopup > menuseparator". That rule doesn't use `!important` so it shouldn't be necessary here.
Attachment #8532936 - Flags: review?(jaws) → review+
Attached image bug1018703.png
I remove the !important from the CSS file and it is not working. From the screenshot, we can see that menu.css has !important mentioned with a padding of 1px.
Flags: needinfo?(jaws)
Ah ok, !important is only found in the osx theme and was introduced by bug 342515. I'm weary of removing it from the osx toolkit theme because it's not obvious what may be relying on it, especially since it's been there for so long.

We can keep the !important there, but please add a comment stating that it is necessary to override the !important from the osx toolkit theme for menuseparators.
Flags: needinfo?(jaws)
Should I submit a new patch with the comment in CSS file? "Necessary to override !important from osx toolkit them for menuseparators"
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> Ah ok, !important is only found in the osx theme and was introduced by bug
> 342515. I'm weary of removing it from the osx toolkit theme because it's not
> obvious what may be relying on it, especially since it's been there for so
> long.

Remove it and see if something breaks? If it's important enough, somebody will notice and complain.
(In reply to Dão Gottwald [:dao] from comment #27)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> > Ah ok, !important is only found in the osx theme and was introduced by bug
> > 342515. I'm weary of removing it from the osx toolkit theme because it's not
> > obvious what may be relying on it, especially since it's been there for so
> > long.
> 
> Remove it and see if something breaks? If it's important enough, somebody
> will notice and complain.

Cool, let's go ahead and remove the !important from the osx menu.css then. Thanks for chiming in :)
Attached patch bug1018703_new.diff (obsolete) — Splinter Review
A new patch removing !important from menu.css in osx and from common.css
Attachment #8533854 - Flags: review?(jaws)
Comment on attachment 8533854 [details] [diff] [review]
bug1018703_new.diff

Review of attachment 8533854 [details] [diff] [review]:
-----------------------------------------------------------------

Please "fold" or merge these two patches together and reupload.
Attachment #8533854 - Flags: review?(jaws) → review+
I accidentally messed up with the patches. I ran hg qfinish -a which I think removed all my patch files. How should I recover to old state and merge the patches.
Since your changesets have not been pushed to a remote server, you can reimport them and fold them doing the following commands:

hg qimport -r tip
hg qpop
*note the name of the file that was popped (should be something like 123456.diff)
hg qimport -r tip
hg qfold 123456.diff
* using the name that was noted above

This will get you a folded patch of both of the changes, and you can upload that patch file.
(In reply to Shubham Jindal from comment #31)
> I accidentally messed up with the patches. I ran hg qfinish -a which I think
> removed all my patch files. How should I recover to old state and merge the
> patches.

Btw, if you have problems like this, you can always ask the #introduction channel on IRC.
Attached patch bug1018703.diffSplinter Review
Attachment #8532936 - Attachment is obsolete: true
Attachment #8533854 - Attachment is obsolete: true
Attachment #8533897 - Flags: review?(jaws)
Comment on attachment 8533897 [details] [diff] [review]
bug1018703.diff

Review of attachment 8533897 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Looks good :)
Attachment #8533897 - Flags: review?(jaws) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/806ccfabfbde
Keywords: checkin-needed
Whiteboard: [good first bug][lang=css] → [good first bug][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/806ccfabfbde
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=css][fixed-in-fx-team] → [good first bug][lang=css]
Target Milestone: --- → Firefox 37
- Verified on Windows 7 64bit and Mac OSX 10.9.5 - the issue is fixed
- Verified on Ubuntu 13.10 32bit - the issue is not fixed: the background does not "reach" up to the separator (from both sides) when hovering over the dropdown's items (Preferences - Applications). 

I've tested on latest Nightly 37.0a1 (buildID: 20150107030217).
(In reply to Camelia Badau, QA [:cbadau] from comment #38)
> - Verified on Windows 7 64bit and Mac OSX 10.9.5 - the issue is fixed
> - Verified on Ubuntu 13.10 32bit - the issue is not fixed: the background
> does not "reach" up to the separator (from both sides) when hovering over
> the dropdown's items (Preferences - Applications). 
> 
> I've tested on latest Nightly 37.0a1 (buildID: 20150107030217).
Thanks for testing, can you file a new bug about this please ?
(In reply to Tim Nguyen [:ntim] from comment #39)
> (In reply to Camelia Badau, QA [:cbadau] from comment #38)
> > - Verified on Windows 7 64bit and Mac OSX 10.9.5 - the issue is fixed
> > - Verified on Ubuntu 13.10 32bit - the issue is not fixed: the background
> > does not "reach" up to the separator (from both sides) when hovering over
> > the dropdown's items (Preferences - Applications). 
> > 
> > I've tested on latest Nightly 37.0a1 (buildID: 20150107030217).
> Thanks for testing, can you file a new bug about this please ?

I filled bug 1119144.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.