1475208 - Font editor: font size range doesn't expand to include out-of-bounds values

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Razvan Caliman [:rcaliman]]

Attachment: upper limit for font-size range input

Steps:

- Paste this in the address bar of a new tab:
data:text/html,<style>div {font-size:20em}</style><div>TEST

- Open the DevTools and inspect the <div> element
- Switch to the Fonts sidebar
- Drag the slider for the font size (20em)

Expected
- Decreasing the value from the slider goes to 19em, 18em and so forth

Actual
- Decreasing the value from the slider goes to 10em. That value is the upper limit for the range for font-size in em units defined internally by the font editor. Other units have different maximum range values.

Out-of-bounds values may come from explicit CSS declarations (ex: font-size: 999px) or from unit conversion using the font editor (50vw -> px). The ranges for various unit types should expand to the upper limit defined by the out-of-bounds value.

As an enhancement: dragging a slider to the upper limit should increment the upper limit allowing the user to drag beyond the initial range for that unit.

Comment 1

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1475208 - (Part 1) Introduce uniqueID to font editor Redux store.

Review commit: https://reviewboard.mozilla.org/r/260350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260350/

Comment 2

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1475208 - (Part 2) Update order of units and upper bounds for font size component.

Review commit: https://reviewboard.mozilla.org/r/260352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260352/

Comment 3

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1475208 - (Part 3) Record upper bounds by unit in font size component.

Review commit: https://reviewboard.mozilla.org/r/260354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260354/

Comment 4

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1475208 - (Part 4) Auto-increment font size when value reaches upper bound.

Review commit: https://reviewboard.mozilla.org/r/260356/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260356/

Comment 5

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1475208 - (Part 5) Add keydown/keyup handlers for font size auto-increment; refine mousedown/mouseup handlers.

Review commit: https://reviewboard.mozilla.org/r/260358/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260358/

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8996110 [details]
Bug 1475208 - (Part 4) Auto-increment font size when value reaches upper bound.

https://reviewboard.mozilla.org/r/260356/#review267420


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/fonts/components/FontPropertyValue.js:100
(Diff revision 1)
>      });
>    }
>  
>    onMouseUp(e) {
> +
> +  startAutoIncrement() {

Error: Parsing error: Unexpected token { [eslint]

Comment 7

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996107 [details]
Bug 1475208 - (Part 1) Introduce uniqueID to font editor Redux store.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260350/diff/1-2/

Comment 8

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996108 [details]
Bug 1475208 - (Part 2) Update order of units and upper bounds for font size component.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260352/diff/1-2/

Comment 9

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996109 [details]
Bug 1475208 - (Part 3) Record upper bounds by unit in font size component.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260354/diff/1-2/

Comment 10

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996110 [details]
Bug 1475208 - (Part 4) Auto-increment font size when value reaches upper bound.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260356/diff/1-2/

Comment 11

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996111 [details]
Bug 1475208 - (Part 5) Add keydown/keyup handlers for font size auto-increment; refine mousedown/mouseup handlers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260358/diff/1-2/

Comment 12

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996110 [details]
Bug 1475208 - (Part 4) Auto-increment font size when value reaches upper bound.

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

Comment 13

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996111 [details]
Bug 1475208 - (Part 5) Add keydown/keyup handlers for font size auto-increment; refine mousedown/mouseup handlers.

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

Comment 14

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1475208 - (Part 6) Cast property values to string before writing to store and stylesheet.

Review commit: https://reviewboard.mozilla.org/r/261156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261156/

Comment 15

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996111 [details]
Bug 1475208 - (Part 5) Add keydown/keyup handlers for font size auto-increment; refine mousedown/mouseup handlers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260358/diff/3-4/

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8996107 [details]
Bug 1475208 - (Part 1) Introduce uniqueID to font editor Redux store.

https://reviewboard.mozilla.org/r/260350/#review268760

::: devtools/client/inspector/fonts/reducers/font-editor.js:43
(Diff revision 2)
>      values: [],
>    },
>    // CSS font properties defined on the selected rule.
>    properties: {},
> -
> +  // Unique identifier for the selected element.
> +  uniqueID: "",

We have used "id" in the past for this purpose. I think we should do so here as well.

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8996108 [details]
Bug 1475208 - (Part 2) Update order of units and upper bounds for font size component.

https://reviewboard.mozilla.org/r/260352/#review268764

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8996109 [details]
Bug 1475208 - (Part 3) Record upper bounds by unit in font size component.

https://reviewboard.mozilla.org/r/260354/#review268768

::: devtools/client/inspector/fonts/components/FontSize.js:25
(Diff revision 2)
>      };
>    }
>  
> +  constructor(props) {
> +    super(props);
> +    this.historicMax = {};

I think we should consider using React component states here.

::: devtools/client/inspector/fonts/components/FontSize.js:28
(Diff revision 2)
> +  constructor(props) {
> +    super(props);
> +    this.historicMax = {};
> +  }
> +
> +  componentWillUnmount() {

I don't think this is necessary since we are only storing a simple object here in historicMax and will be easily destroyed when the component unmounts. This would be different if we were trying to remove any event listener subscription.

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8997362 [details]
Bug 1475208 - (Part 6) Cast property values to string before writing to store and stylesheet.

https://reviewboard.mozilla.org/r/261156/#review268772

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8996110 [details]
Bug 1475208 - (Part 4) Auto-increment font size when value reaches upper bound.

https://reviewboard.mozilla.org/r/260356/#review268774

::: devtools/client/inspector/fonts/components/FontPropertyValue.js:35
(Diff revision 3)
>    constructor(props) {
>      super(props);
> +    // Interval ID for the auto-increment operation.
> +    this.interval = null;
> +    // Milliseconds between auto-increment interval iterations.
> +    this.intervalDelay = 300;

We should move this to be a constant like UNITS.

::: devtools/client/inspector/fonts/components/FontPropertyValue.js:37
(Diff revision 3)
> +    // Interval ID for the auto-increment operation.
> +    this.interval = null;
> +    // Milliseconds between auto-increment interval iterations.
> +    this.intervalDelay = 300;
>      this.state = {
>        // Whether the user is dragging the slider thumb or pressing on the numeric stepper.

We should also add value: null to make it clear that we have this value component state.

::: devtools/client/inspector/fonts/components/FontPropertyValue.js:51
(Diff revision 3)
>    }
>  
> +  componentDidUpdate(prevProps, prevState) {
> +    // Clear the auto-increment interval if interactive state changed from true to false.
> +    if (prevState.interactive && !this.state.interactive) {
> +      this.stopAutoIncrement();

I am wondering if we should guard against the interval timer by stopping the auto increment in a componentWillUnmount as well.

::: devtools/client/inspector/fonts/components/FontPropertyValue.js:105
(Diff revision 3)
>    onUnitChange(e) {
>      this.props.onChange(this.props.name, this.props.value, this.props.unit,
>         e.target.value);
> +    // Reset internal state value and wait for converted value from props.
> +    this.setState((prevState) => {
> +      return { ...prevState, value: null };

Can we move prevState and value: null to separate lines to make it more clear when parsing.

return {
  ...prevState,
  value: null,
};

::: devtools/client/inspector/fonts/components/FontPropertyValue.js:145
(Diff revision 3)
> +   * @param {Boolean} isInteractive
> +   *        Whether to mark the interactive state on or off.
> +   */
> +  toggleInteractiveState(isInteractive) {
> +    this.setState((prevState, props) => {
> +      return { ...prevState, interactive: isInteractive };

Same as above.

::: devtools/client/inspector/fonts/components/FontPropertyValue.js:160
(Diff revision 3)
> +   *        Numeric property value.
> +   */
> +  updateValue(value) {
> +    this.props.onChange(this.props.name, value, this.props.unit);
> +    this.setState((prevState) => {
> +      return { ...prevState, value };

Same as above.

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8996111 [details]
Bug 1475208 - (Part 5) Add keydown/keyup handlers for font size auto-increment; refine mousedown/mouseup handlers.

https://reviewboard.mozilla.org/r/260358/#review268808

::: devtools/client/inspector/fonts/components/FontPropertyValue.js:12
(Diff revision 4)
>  const { PureComponent } = require("devtools/client/shared/vendor/react");
>  const dom = require("devtools/client/shared/vendor/react-dom-factories");
>  const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
>  
> +// Key codes for arrow keys.
> +const ARROW_UP = 38;

We also have a keycodes.js library that will provide these constants that you might want to take advantage.

Comment 22

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996111 [details]
Bug 1475208 - (Part 5) Add keydown/keyup handlers for font size auto-increment; refine mousedown/mouseup handlers.

https://reviewboard.mozilla.org/r/260358/#review268928

Comment 23

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996109 [details]
Bug 1475208 - (Part 3) Record upper bounds by unit in font size component.

https://reviewboard.mozilla.org/r/260354/#review268930

Comment 24

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996110 [details]
Bug 1475208 - (Part 4) Auto-increment font size when value reaches upper bound.

https://reviewboard.mozilla.org/r/260356/#review268774

> I am wondering if we should guard against the interval timer by stopping the auto increment in a componentWillUnmount as well.

Good catch! It's unlikely to happen without also triggering "blur" (which stops the auto-increment), but an extra guard won't hurt.

Comment 25

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996109 [details]
Bug 1475208 - (Part 3) Record upper bounds by unit in font size component.

https://reviewboard.mozilla.org/r/260354/#review268768

> I think we should consider using React component states here.

`this.historicMax` is set during `render()`. Using React component state (and setting it during `render()`) would cause needles re-renders. The alternative while still using React compoent state might be to use `getDerivedStateFromProps()` https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops but I don't think the added complexity is justified in this case.

Comment 26

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996107 [details]
Bug 1475208 - (Part 1) Introduce uniqueID to font editor Redux store.

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

Comment 27

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996108 [details]
Bug 1475208 - (Part 2) Update order of units and upper bounds for font size component.

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

Comment 28

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996109 [details]
Bug 1475208 - (Part 3) Record upper bounds by unit in font size component.

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

Comment 29

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996110 [details]
Bug 1475208 - (Part 4) Auto-increment font size when value reaches upper bound.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260356/diff/3-4/

Comment 30

[Razvan Caliman [:rcaliman]]

Comment on attachment 8996111 [details]
Bug 1475208 - (Part 5) Add keydown/keyup handlers for font size auto-increment; refine mousedown/mouseup handlers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260358/diff/4-5/

Comment 31

[Razvan Caliman [:rcaliman]]

Comment on attachment 8997362 [details]
Bug 1475208 - (Part 6) Cast property values to string before writing to store and stylesheet.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261156/diff/1-2/

Comment 32

[Razvan Caliman [:rcaliman]]

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e1542dad1be
(Part 1) Introduce uniqueID to font editor Redux store. r=gl
https://hg.mozilla.org/integration/autoland/rev/b65adfe68eaf
(Part 2) Update order of units and upper bounds for font size component. r=gl
https://hg.mozilla.org/integration/autoland/rev/8c8ef5ecd85f
(Part 3) Record upper bounds by unit in font size component. r=gl
https://hg.mozilla.org/integration/autoland/rev/33b3f88d4232
(Part 4) Auto-increment font size when value reaches upper bound. r=gl
https://hg.mozilla.org/integration/autoland/rev/d473e5317676
(Part 5) Add keydown/keyup handlers for font size auto-increment; refine mousedown/mouseup handlers. r=gl
https://hg.mozilla.org/integration/autoland/rev/2ad148a42f6c
(Part 6) Cast property values to string before writing to store and stylesheet. r=gl

Comment 34

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7e1542dad1be
https://hg.mozilla.org/mozilla-central/rev/b65adfe68eaf
https://hg.mozilla.org/mozilla-central/rev/8c8ef5ecd85f
https://hg.mozilla.org/mozilla-central/rev/33b3f88d4232
https://hg.mozilla.org/mozilla-central/rev/d473e5317676
https://hg.mozilla.org/mozilla-central/rev/2ad148a42f6c

Comment 35

[Timea Zsoldos [:tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 63.0a1 (2018.08.05) on Windows 10 x64.
I can confirm this issue is fixed, I verified using Firefox 63.0b5 on Ubuntu 16.04 x64, Windows 10 x64 and Mac OS X 10.13.