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)

Product:
DevTools
Component:
Inspector: Rules
Type:
defect

Tracking

(firefox54 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- verified

People

(Reporter: arni2033, Assigned: Towkir, Mentored)

Details

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

Attachments

(4 files, 2 obsolete files)

Capture.PNG
22.16 KB, image/png
Details
rulevew-swatch-light.gif
101.95 KB, image/gif
Details
rulevew-swatch-dark.gif
92.54 KB, image/gif
Details
ruleview-swatch.patch
552 bytes, patch
Towkir
: review+
Details | Diff | Splinter Review 
>>>   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)}
No longer blocks: 1277113
Component: Untriaged → Developer Tools
Component: Developer Tools → Developer Tools: CSS Rules Inspector
Attached image Capture.PNG — 
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.
Marking as WORKSFORME for now. Feel free to re-open with a screenshot.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
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)
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 → ---
Status: REOPENED → NEW
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
Attached patch ruleview-swatch.patch (obsolete) — — Splinter Review 
Looks like Tim is not reviewing for now.
Hi Patrick, can you have a look at this ?
Thanks
Attachment #8825117 - Flags: review?(pbrosset)
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+
(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.
Attachment #8825117 - Flags: review+
Looks like the patch is good to go for now. Is it Tim ?
Flags: needinfo?(ntim.bugs)
(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)
(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)
(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)
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
Attached image rulevew-swatch-dark.gif — 
Here is the result on dark theme.
Flags: needinfo?(pbrosset)
Flags: needinfo?(ntim.bugs)
(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)
Attached patch ruleview-swatch.patch (obsolete) — — Splinter Review 
We need to see the results from OSX too.
Attachment #8825117 - Attachment is obsolete: true
Attachment #8832324 - Flags: review?(pbrosset)
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+
Alright, here is it. I did r+ myself.
Attachment #8832324 - Attachment is obsolete: true
Attachment #8833272 - Flags: review+
I hope it is good to go now
Keywords: checkin-needed
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
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
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]
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: