Units cycling with shift+click should persists and change the value in Style editor

VERIFIED FIXED in Firefox 48

Status

defect
P2
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: nchevobbe, Assigned: nchevobbe)

Tracking

48 Branch
Firefox 48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 verified)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(2 attachments)

Posted file angle.html
For color and angle values, we can cycle through the units ( rgb -> hex -> hsl -> name , deg -> rad -> grad -> turn ) with shift + click.
Those unit changes don't persist though.

STR : 
1. Open the attached file
2. Open inspector
3. Select the <body> node
4. shift + click on the `rgba(183,222,237,1)` color swatch to the hex value (`#b7deed`)
5. select the <html> node
6. select the <body> node


Expected result : 
The first color of the gradient should be `#b7deed`.


Actual result : 
The first color of the gradient is `rgba(183,222,237,1)`, the original value.
Assignee

Updated

3 years ago
Has STR: --- → yes
Component: Developer Tools → Developer Tools: CSS Rules Inspector
Version: 47 Branch → 48 Branch
Assignee

Updated

3 years ago
Assignee: nobody → chevobbe.nicolas
Triaging (filter on CLIMBING SHOES).
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Assignee

Comment 2

3 years ago
So I thought a little more about this one and there is something that should be taken care of. In the option panel, you can chose how colours should be displayed. It can be "authored", i.e. like it is declared by the user in the rule, or it can be one of hex, hsl, rgba, name.

Maybe we could only persist value through shift+click if the pref is set to "authored". But it can be confusing to both persist and not, only depending of the state of a preference.

What do you think about it ?
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(hholmes)
I think we should always persist, whether it was authored or not. I think that the changing would be more confusing than anything else, like you've already mentioned.
Flags: needinfo?(hholmes)

Comment 4

3 years ago
We should persist in any case. Editing the unit manually without shift click does persist, and we should do the same with shift click
Flags: needinfo?(ntim.bugs)
Assignee

Comment 5

3 years ago
Add an 'unit-change' event fired when shift+click on color and angle swatches.
Add a listener on this event in text-property-editor.js to call the same function
that's called when tooltip edit is commited to persist the new unit.
Edit some tests to adapt to this new behaviour and create some tests to make sure
the value obtained via shift+click are actually persisted.

Review commit: https://reviewboard.mozilla.org/r/43039/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43039/
Attachment #8736013 - Flags: review?(mratcliffe)

Updated

3 years ago
Status: NEW → ASSIGNED
Assignee

Comment 6

3 years ago
Comment on attachment 8736013 [details]
MozReview Request: Bug 1259559 - Units cycling with shift+click persists value in Style editor. r=miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43039/diff/1-2/
Comment on attachment 8736013 [details]
MozReview Request: Bug 1259559 - Units cycling with shift+click persists value in Style editor. r=miker

https://reviewboard.mozilla.org/r/43039/#review39733

Looks spot on... r+ assuming a green try.

::: devtools/client/inspector/rules/test/browser_rules_cycle-color.js:18
(Diff revision 2)
>      }
> +    span {
> +      color: blue;
> +    }
>    </style>
> -  Test cycling color types in the rule view!
> +  <body><span>Test</span>cycling color types in the rule view!</body>

Nit: Add a space after the closing span.
Attachment #8736013 - Flags: review?(mratcliffe) → review+
Assignee

Comment 8

3 years ago
Comment on attachment 8736013 [details]
MozReview Request: Bug 1259559 - Units cycling with shift+click persists value in Style editor. r=miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43039/diff/2-3/
Assignee

Comment 9

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ddcc57f9baf

The TRY run is over and i think everything is good

Errors known : 

dt8 on Windows 7 debug : browser_webconsole_bug_632817.js https://bugzilla.mozilla.org/show_bug.cgi?id=1244707

dt3 on Linux debug| dt2 on Linux 64 debug : No bug filed, but appears in another TRY run without my patch ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=7db58284a44ef0cbd4518ae34419a66ed1f82b35&selectedJob=18769152 )
Keywords: checkin-needed

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6d91363a7eac
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Comment 12

3 years ago
I have reproduced this bug with nightly 48.0a1 (2016-03-24) on Windows 10, 64 bit!

The Bug's fix is now verified on Latest Beta 48.0b6

Build ID 	20160706215822
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

[bugday-20160713]

Updated

3 years ago
Status: RESOLVED → VERIFIED
Depends on: 1284852

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.