Closed Bug 1475208 Opened Last year Closed Last year

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

Categories

(DevTools :: Inspector: Fonts, defect, P2)

defect

Tracking

(firefox63 verified)

VERIFIED FIXED
Firefox 63
Tracking Status
firefox63 --- verified

People

(Reporter: rcaliman, Assigned: rcaliman)

References

Details

Attachments

(7 files)

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 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 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.
Attachment #8996107 - Flags: review?(gl) → review+
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
Attachment #8996108 - Flags: review?(gl) → review+
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.
Attachment #8996109 - Flags: review?(gl) → review+
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
Attachment #8997362 - Flags: review?(gl) → review+
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.
Attachment #8996110 - Flags: review?(gl) → review+
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.
Attachment #8996111 - Flags: review?(gl) → review+
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 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 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 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.
Pushed by rcaliman@mozilla.com:
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
QA Contact: catalin.sasca
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.