1166956 - Incrementing from 0 to 1 should provide units

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

STR:

* add a new css property to a rule eg 'max-width', set an initial value of zero. 
* with focus still on the 0, arrow up

Expected: 0 is unit-less in CSS, we should ensure that the new value is valid by guessing at what unit to use ( probably px )

Actual: new property value becomes 1 with no unit, so the new value is invalid.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: bug1166956.wip.patch

Just a work in progress patch, I plan to add more tests.
Below, some background/explanations on the patch + 1 open question.

:tromey : can you give me your feedback on the overall direction of the patch, as well as your opinion about the open question below?

(:pbro : ni? for you to try the patch) 

# patch background

According to css dimensions specs [1], numbers in CSS may have the following dimensions : distance, angle, duration, frequence, resolution. 
(will omit resolution here as it's only used for media queries)

On the other hand, CSS properties can use :
- no dimension (z-index)
- always the same dimension (margin uses only distances)
- mix of dimensions (linear-gradient uses angles and distances)

Ideally the goal is to be able to guess a correct unit whenever we are trying to increment a unit-less "0".
For each dimension we can pick a default compatible unit :
- distance: px
- angle: deg
- duration: s
- frequence: Hz

The inplace-editor could check the validity of a value, and therefore test different options before applying it. When the editor has to increment "0", we try "is 1px valid?", "is 1deg valid?" ... until finding a valid unit. We rely on `domUtils.cssPropertyIsValid(propName, propValue)` to perform this check.

This allows us to correctly guess the unit in any context. For instance in `background: linear-gradient(0, red 0, blue 0);` each "0" is correctly incremented.

# Open question : guess unit only for distance?

According to CSS length specs [2], the unit is optional for a "0" value only for distances. 

Firefox and IE/Edge consider 0 as an invalid angle/durationfrequence. 
Chrome and Safari consider 0 as a valid angle/duration/frequence.

We could guess the unit only for distances, ie. only check "is 1px valid?". On the other hand, supporting other dimensions comes almost for free here and could still be useful.

[1] https://www.w3.org/TR/css3-values/#dimensions
[2] https://www.w3.org/TR/css3-values/#lengths

Comment 2

[Patrick Brosset <:pbro>]

Tested locally, I like it very much! This seems to work really well.

(In reply to Julian Descottes [:jdescottes] from comment #1)
> # Open question : guess unit only for distance?
> 
> According to CSS length specs [2], the unit is optional for a "0" value only
> for distances. 
> 
> Firefox and IE/Edge consider 0 as an invalid angle/durationfrequence. 
> Chrome and Safari consider 0 as a valid angle/duration/frequence.
> 
> We could guess the unit only for distances, ie. only check "is 1px valid?".
> On the other hand, supporting other dimensions comes almost for free here
> and could still be useful.
I think we should support other dimensions for the simple reason that people might sometimes forget to add the unit. The fact that Firefox doesn't support a linear-gradient with angle 0 doesn't mean that people aren't going to try and add one. And when they do, being able to simply change from 0 to 1deg by pressing one arrow key will be very useful.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: bug1166956.wip.patch (check if range is defined)

Thanks for the feedback Patrick! I feel the same way about supporting the other dimensions.

Updating my patch as the previous one failed some existing tests.

Comment 4

[Tom Tromey :tromey]

Comment on attachment 8718376 [details] [diff] [review]
bug1166956.wip.patch (check if range is defined)

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

This approach looks fine to me.

There's also domutils.cssPropertySupportsType, but your approach seems more robust.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: bug1166956.v1.patch

Thanks for the feedback!
Added more test cases to browser_rules_edit-property-increments.js.

try push : https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b3d43ef0010

Comment 6

[Julian Descottes [:jdescottes]]

FWIW, with the added test cases browser_rules_edit-property-increments.js takes around 24 seconds to run on Linux32 debug. Still far away from the 45 seconds timeout threshold.

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8718633 [details] [diff] [review]
bug1166956.v1.patch

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

Looks good.

Would it be possible to add a test case covering the "Hz" case?  I think all other cases are accounted for.

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to Tom Tromey :tromey from comment #7)
> Comment on attachment 8718633 [details] [diff] [review]
> bug1166956.v1.patch
> 
> Review of attachment 8718633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> Would it be possible to add a test case covering the "Hz" case?  I think all
> other cases are accounted for.

Thanks for the review! 

Actually, even though the frequency dimension is in the CSS3 specs, it is not supported by any property yet. After thinking more about this, if it's not used and not tested, we might as well remove it for now from the implementation?

Comment 9

[Tom Tromey :tromey]

(In reply to Julian Descottes [:jdescottes] from comment #8)

> Actually, even though the frequency dimension is in the CSS3 specs, it is
> not supported by any property yet. After thinking more about this, if it's
> not used and not tested, we might as well remove it for now from the
> implementation?

Yeah, in this case I think that would be good.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: bug1166956.v2.patch

Great, frequency is now gone! 
Carry over r+ 

Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=2306bac7e513

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: bug1166956.v3.patch (rebased)

Rebased, carry over r+.

Comment 12

[Julian Descottes [:jdescottes]]

Can you push attachment 8719332 [details] [diff] [review] ?

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/fb1b8c571e95

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb1b8c571e95