Bug 148351

Summary: Web Inspector: Picking unchanged for font-size does not reset back to the unchanged value
Product: WebKit Reporter: Timothy Hatcher <timothy>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, drousso, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 147563    
Attachments:
Description Flags
Video
none
Patch
bburg: review+, bburg: commit-queue-
Patch none

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.