Bug 148351 - Web Inspector: Picking unchanged for font-size does not reset back to the unchanged value
Summary: Web Inspector: Picking unchanged for font-size does not reset back to the unc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 147563
  Show dependency treegraph
 
Reported: 2015-08-21 20:33 PDT by Timothy Hatcher
Modified: 2015-09-15 21:22 PDT (History)
9 users (show)

See Also:


Attachments
Video (6.94 MB, video/quicktime)
2015-08-21 20:33 PDT, Timothy Hatcher
no flags Details
Patch (5.57 KB, patch)
2015-09-11 22:30 PDT, Devin Rousso
bburg: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
Patch (5.45 KB, patch)
2015-09-15 20:12 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2015-08-21 20:33:55 PDT
Created attachment 259704 [details]
Video

Steps:

1)Change font-size.
2)Pick unchanged from the menu

Results: Stays at the edited value. Note the rule icons reverts to the unchanged icon.
Expected: Reverts to the original computed value

See video.
Comment 1 Radar WebKit Bug Importer 2015-09-08 16:05:26 PDT
<rdar://problem/22619595>
Comment 2 Devin Rousso 2015-09-11 22:30:59 PDT
Created attachment 261045 [details]
Patch
Comment 3 BJ Burg 2015-09-15 10:43:30 PDT
Comment on attachment 261045 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=261045&action=review

r=me. Please change the flag setter's name, though.

I would like us to work towards storing less state in the visual style editor's DOM elements.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:115
> +            if (value || this._updatedValues.placeholder)

There are lots of places where we have fallback values, can these be extracted into well-named getters?

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:235
> +    _markNumberInputEditable(flag)

This should match the name of the flag, i.e., this._setNumberInputIsEditable(flag). There's no reason for it to differ, and it reads better.
Comment 4 Devin Rousso 2015-09-15 20:05:47 PDT
(In reply to comment #3)
> Comment on attachment 261045 [details]

> I would like us to work towards storing less state in the visual style
> editor's DOM elements.
I would agree, but I think that that should be its own bug.  I can modify all the visual style classes to use this methodology and will do so soon (my homework sucks).

> > Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:115
> > +            if (value || this._updatedValues.placeholder)
> 
> There are lots of places where we have fallback values, can these be
> extracted into well-named getters?

I think that, for now, it is easier and clearer to use these fallbacks.  I've thought a bit about how to move away from storing the state in the DOM and I think that that change would resolve many of these cases.
Comment 5 Devin Rousso 2015-09-15 20:12:18 PDT
Created attachment 261289 [details]
Patch
Comment 6 WebKit Commit Bot 2015-09-15 21:22:36 PDT
Comment on attachment 261289 [details]
Patch

Clearing flags on attachment: 261289

Committed r189845: <http://trac.webkit.org/changeset/189845>
Comment 7 WebKit Commit Bot 2015-09-15 21:22:40 PDT
All reviewed patches have been landed.  Closing bug.