Closed
Bug 1327691
Opened 7 years ago
Closed 7 years ago
CSS - Circles in ruleview (.ruleview-swatch) are misplaced by 0.1em
Categories
(DevTools :: Inspector: Rules, defect, P3)
DevTools
Inspector: Rules
Tracking
(firefox54 verified)
VERIFIED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | verified |
People
(Reporter: arni2033, Assigned: Towkir, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [lang=css])
Attachments
(4 files, 2 obsolete files)
>>> My Info: Win7_64, Nightly 49, 32bit, ID 20160526082509 STR_1: 1. Open url [1] 2. Open devtools -> inspector -> rules, inspect <body> element 3. Look at the swatches in ruleview (increase zoom level in devtools to see better) AR: Swatches are noticeably misplaced, on HiDPI (125%) it's noticeable even w/o changing zoom level ER: Swatches should be placed correctly Note: This happens because width and height of .ruleview-swatch is defined as "0.9em", while background-size of all swatches is defined as "1em auto". > [1] data:text/html,<style>body{filter:none;color:transparent;animation:ease;transform:rotate(0deg)}
Component: Developer Tools → Developer Tools: CSS Rules Inspector
Comment 1•7 years ago
|
||
What do you mean by misplaced? I've attached a screenshot of what it looks like for me (tested on 51 and 53, they look the same in both). It would help if you could also attach a screenshot.
Comment 2•7 years ago
|
||
Marking as WORKSFORME for now. Feel free to re-open with a screenshot.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
Comment 3•7 years ago
|
||
Your screenshot, that was taken on HiDPI, perfectly describes the bug. Icons in all circles are misaligned (not placed in the center). Bug 1327981 is related, please add it in "See also" list.
Flags: needinfo?(pbrosset)
Comment 4•7 years ago
|
||
Ok thanks for confirming. So the problem is the icons inside the circle that are slightly offset to the right.
Status: RESOLVED → REOPENED
Flags: needinfo?(pbrosset)
Priority: -- → P3
Resolution: WORKSFORME → ---
Comment 5•7 years ago
|
||
https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/rules.css#369-370 Change this to 1em
Keywords: good-first-bug
Whiteboard: [lang=css]
Updated•7 years ago
|
Status: REOPENED → NEW
Assignee | ||
Comment 6•7 years ago
|
||
Hi Tim, According to comment 5 making you a mentor here. Will submit a patch soon, hope you will take care about that :)
Assignee: nobody → 3ugzilla
Mentor: ntim.bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•7 years ago
|
||
Looks like Tim is not reviewing for now. Hi Patrick, can you have a look at this ? Thanks
Attachment #8825117 -
Flags: review?(pbrosset)
Comment 8•7 years ago
|
||
Comment on attachment 8825117 [details] [diff] [review] ruleview-swatch.patch Review of attachment 8825117 [details] [diff] [review]: ----------------------------------------------------------------- Tested on Windows 10. It does indeed seem to address the problem nicely. I wonder why it was 0.9em to begin with. Tim: any idea about the history for this thing? Are we likely to re-surface another problem by bumping this to 1em? In any case, simple enough CSS change that can get a quick R+. But let's wait for Tim's feedback on this to land it.
Attachment #8825117 -
Flags: review?(pbrosset) → review+
Comment 9•7 years ago
|
||
(In reply to Patrick Brosset <:pbro> from comment #8) > Comment on attachment 8825117 [details] [diff] [review] > ruleview-swatch.patch > > Review of attachment 8825117 [details] [diff] [review]: > ----------------------------------------------------------------- > > Tested on Windows 10. It does indeed seem to address the problem nicely. I > wonder why it was 0.9em to begin with. > Tim: any idea about the history for this thing? Are we likely to re-surface > another problem by bumping this to 1em? > In any case, simple enough CSS change that can get a quick R+. But let's > wait for Tim's feedback on this to land it. Looking at hg blame, the dimensions were changed from 1em to .9em by bug 1154059, but on Windows, it seems like bug 1154059 is more visible with .9em than with 1em. As for OSX, I've noticed no change. I don't know about linux though. If bug 1154059 occurs again on linux, I think using border instead of box-shadow for the border would be a better fix.
Updated•7 years ago
|
Attachment #8825117 -
Flags: review+
Assignee | ||
Comment 10•7 years ago
|
||
Looks like the patch is good to go for now. Is it Tim ?
Flags: needinfo?(ntim.bugs)
Comment 11•7 years ago
|
||
(In reply to [:Towkir] Ahmed from comment #10) > Looks like the patch is good to go for now. Is it Tim ? Have you tested if bug 1154059 occurs on Linux with the patch applied ? If it doesn't occur, feel free to mark as checkin-needed.
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #11) > Have you tested if bug 1154059 occurs on Linux with the patch applied ? If > it doesn't occur, feel free to mark as checkin-needed. Sorry, I don't have a HiDPI monitor and which I have is not that good, So I can't be sure about that and I don't want to be in doubt about this. Can we get help from the qa team or anyone else (you know) here ? Thanks
Flags: needinfo?(ntim.bugs)
Comment 13•7 years ago
|
||
(In reply to [:Towkir] Ahmed from comment #12) > (In reply to Tim Nguyen :ntim from comment #11) > > Have you tested if bug 1154059 occurs on Linux with the patch applied ? If > > it doesn't occur, feel free to mark as checkin-needed. > Sorry, I don't have a HiDPI monitor and which I have is not that good, So I > can't be sure about that and I don't want to be in doubt about this. It doesn't seem HiDPI specific (the screenshot in that bug is normal DPI). Just check if you can reproduce on your machine, and it should be enough really. Someone will file a bug if not.
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 14•7 years ago
|
||
I did test this on linux, looks good. I am attaching screencast for both light and dark theme, Before landing this, can we remove that negative 1px margin-top ?? See what changes when I toggle the margin. Let me know what you guys think
Assignee | ||
Comment 15•7 years ago
|
||
Here is the result on dark theme.
Flags: needinfo?(pbrosset)
Flags: needinfo?(ntim.bugs)
Comment 16•7 years ago
|
||
(In reply to [:Towkir] Ahmed from comment #14) > Created attachment 8832048 [details] > rulevew-swatch-light.gif > > I did test this on linux, looks good. I am attaching screencast for both > light and dark theme, > Before landing this, can we remove that negative 1px margin-top ?? > See what changes when I toggle the margin. Looks good with the margin change.
Flags: needinfo?(pbrosset)
Flags: needinfo?(ntim.bugs)
Assignee | ||
Comment 17•7 years ago
|
||
We need to see the results from OSX too.
Attachment #8825117 -
Attachment is obsolete: true
Attachment #8832324 -
Flags: review?(pbrosset)
Comment 18•7 years ago
|
||
Comment on attachment 8832324 [details] [diff] [review] ruleview-swatch.patch Review of attachment 8832324 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! See my comment below. I'll R+ this now. Feel free to attach a new patch and mark it as R+ yourself. ::: devtools/client/themes/rules.css @@ -371,3 @@ > vertical-align: middle; > - /* align the swatch with its value */ > - margin-top: -1px; I tested this on Windows and Mac. It looks fine on Windows, but on Mac, the swatches are a little bit too low I think. I would like to keep this negative margin-top.
Attachment #8832324 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 19•7 years ago
|
||
Alright, here is it. I did r+ myself.
Attachment #8832324 -
Attachment is obsolete: true
Attachment #8833272 -
Flags: review+
Comment 21•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a9f30b32cfc CSS - Circles in ruleview (.ruleview-swatch) are now placed properly. r=pbro
Keywords: checkin-needed
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5a9f30b32cfc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 23•7 years ago
|
||
Screenshots of the change: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=94079d43835ff229dce0f9b226cedd64c6aaef60&newProject=mozilla-central&newRev=09b28b955a478f5a63b96db165a7974b367e14c3&filter=%5E%28win%7Cosx%29.*devtools It seems like it made the swatches larger, not only re-positioning them. No reply necessary if it's fine now.
Comment 24•7 years ago
|
||
I have reproduced this bug with Nightly 53.0a1 (2017-01-01) on Windows 10, 64 bit! The bug's fix is now verified on Latest Beta 54.0b7 Build ID 20170511130324 User Agent Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0 [testday-20170512]
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•