Closed
Bug 1018703
Opened 11 years ago
Closed 10 years ago
in-content preferences: background margin issue when hovering on dropdown items with separators
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
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.
Comment 1•10 years ago
|
||
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]
Comment 2•10 years ago
|
||
Also, while we're there, the separator border-color should be changed to #c1c1c1 to match the latest specs.
Assignee | ||
Comment 3•10 years ago
|
||
Hi Tim,
I don't have source directory inside mozilla-central. Did I miss something while building the source code?
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Comment 5•10 years ago
|
||
(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.
Comment 6•10 years ago
|
||
(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.
Assignee | ||
Comment 7•10 years ago
|
||
I have removed the margins as you can see from the Console. But the background has not yet filled up till separator.
Comment 8•10 years ago
|
||
(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 ?
Assignee | ||
Comment 9•10 years ago
|
||
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.
Comment 10•10 years ago
|
||
(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 :/
Assignee | ||
Comment 11•10 years ago
|
||
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. :)
Comment 12•10 years ago
|
||
(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 ?
Assignee | ||
Comment 13•10 years ago
|
||
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.
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8532926 -
Flags: review?(ntim007)
Comment 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
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?
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8532926 -
Attachment is obsolete: true
Attachment #8532931 -
Flags: review?(jaws)
Attachment #8532931 -
Flags: feedback?(ntim007)
Comment 19•10 years ago
|
||
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+
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8532931 -
Attachment is obsolete: true
Attachment #8532931 -
Flags: review?(jaws)
Attachment #8532936 -
Flags: review?(jaws)
Attachment #8532936 -
Flags: feedback?(ntim007)
Updated•10 years ago
|
Attachment #8532936 -
Flags: feedback?(ntim007) → feedback+
Updated•10 years ago
|
Assignee: nobody → shubhamjindal18
Status: NEW → ASSIGNED
Assignee | ||
Comment 21•10 years ago
|
||
Just out of curiosity, why did we actually place margin above padding? And can you suggest me a new bug..:)
Comment 22•10 years ago
|
||
(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 23•10 years ago
|
||
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+
Assignee | ||
Comment 24•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(jaws)
Comment 25•10 years ago
|
||
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)
Assignee | ||
Comment 26•10 years ago
|
||
Should I submit a new patch with the comment in CSS file? "Necessary to override !important from osx toolkit them for menuseparators"
Comment 27•10 years ago
|
||
(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.
Comment 28•10 years ago
|
||
(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 :)
Assignee | ||
Comment 29•10 years ago
|
||
A new patch removing !important from menu.css in osx and from common.css
Attachment #8533854 -
Flags: review?(jaws)
Comment 30•10 years ago
|
||
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+
Assignee | ||
Comment 31•10 years ago
|
||
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.
Comment 32•10 years ago
|
||
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.
Comment 33•10 years ago
|
||
(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.
Assignee | ||
Comment 34•10 years ago
|
||
Attachment #8532936 -
Attachment is obsolete: true
Attachment #8533854 -
Attachment is obsolete: true
Attachment #8533897 -
Flags: review?(jaws)
Comment 35•10 years ago
|
||
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+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 36•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [good first bug][lang=css] → [good first bug][lang=css][fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=css][fixed-in-fx-team] → [good first bug][lang=css]
Target Milestone: --- → Firefox 37
Comment 38•10 years ago
|
||
- 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).
Comment 39•10 years ago
|
||
(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 ?
Comment 40•10 years ago
|
||
(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.
Description
•