Bug 199130 - Web Inspector: Styles: curly brace incorrectly added after completed value inside unclosed quote
Summary: Web Inspector: Styles: curly brace incorrectly added after completed value in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-22 15:31 PDT by Nikita Vasilyev
Modified: 2019-07-06 19:09 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.31 KB, patch)
2019-06-22 15:38 PDT, Nikita Vasilyev
hi: review+
hi: commit-queue-
Details | Formatted Diff | Diff
Patch (2.29 KB, patch)
2019-07-06 18:27 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2019-06-22 15:31:49 PDT
Steps:
1. Open https://webkit.org
2. Inspect <body>
3. Add `font-family: "Helv` CSS property to the Style Attribute rule.
4. Press Tab or Enter.

Actual:

    font-family: "Helvetica}"

Expected:
    
    font-family: "Helvetica"

Notes:
This is similar to https://bugs.webkit.org/show_bug.cgi?id=199090.
Weirdly, for this case "}" gets added by WI.tokenizeCSSValue when it encounters unbalanced quotes.
Comment 1 Radar WebKit Bug Importer 2019-06-22 15:33:08 PDT
<rdar://problem/52023534>
Comment 2 Nikita Vasilyev 2019-06-22 15:38:51 PDT
Created attachment 372690 [details]
Patch
Comment 3 Devin Rousso 2019-07-05 12:20:31 PDT
Comment on attachment 372690 [details]
Patch

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

r=me, even though I have questions, the logic looks fine to me.  Please address my questions before landing.

> Source/WebInspectorUI/ChangeLog:9
> +        `}` gets added by WI.tokenizeCSSValue (CodeMirror CSS tokenizer) when it encounters unbalanced quotes.

It's worth mentioning that `WI.tokenizeCSSValue` is called by `_renderValue`, not in `spreadsheetTextFieldDidCommit`.  That initially confused me.

> Source/WebInspectorUI/ChangeLog:10
> +        Fix unbalanced quotes by rendering the value from the model, not the view.

Why does the model have the "correct" value, but the view doesn't?  Every time you edit the view, it updates the model with the corresponding text, so the model and view should be in sync.  Are you saying that they're not?  If so, why?  If not, what is actually changed/fixed by using the model?
Comment 4 Nikita Vasilyev 2019-07-05 14:03:38 PDT
(In reply to Devin Rousso from comment #3)
> Comment on attachment 372690 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372690&action=review
> 
> r=me, even though I have questions, the logic looks fine to me.  Please
> address my questions before landing.
> 
> > Source/WebInspectorUI/ChangeLog:9
> > +        `}` gets added by WI.tokenizeCSSValue (CodeMirror CSS tokenizer) when it encounters unbalanced quotes.
> 
> It's worth mentioning that `WI.tokenizeCSSValue` is called by
> `_renderValue`, not in `spreadsheetTextFieldDidCommit`.  That initially
> confused me.
> 
> > Source/WebInspectorUI/ChangeLog:10
> > +        Fix unbalanced quotes by rendering the value from the model, not the view.
> 
> Why does the model have the "correct" value, but the view doesn't?  Every
> time you edit the view, it updates the model with the corresponding text, so
> the model and view should be in sync.  Are you saying that they're not?  If
> so, why?  If not, what is actually changed/fixed by using the model?

When I said the view, I really meant the DOM.

After entering CSS value `"Helvetica` this is what happens:

    _handleValueChange()
    {
        this._property.rawValue = this._valueElement.textContent.trim();
    }

this._valueElement.textContent is "Helvetica.
this._property.rawValue setter fixes unbalanced quotes, so the value becomes "Helvetica":

    set rawValue(value)
    {
        ...
        let suffix = WI.CSSCompletions.completeUnbalancedValue(value);
        if (suffix)
            value += suffix;
Comment 5 Nikita Vasilyev 2019-07-06 18:27:14 PDT
Created attachment 373589 [details]
Patch
Comment 6 WebKit Commit Bot 2019-07-06 19:09:35 PDT
Comment on attachment 373589 [details]
Patch

Clearing flags on attachment: 373589

Committed r247196: <https://trac.webkit.org/changeset/247196>
Comment 7 WebKit Commit Bot 2019-07-06 19:09:36 PDT
All reviewed patches have been landed.  Closing bug.