Bug 46367 – CSSParser: Enable rule selector source range extraction

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexander Pavlov (apavlov)]

CSSStyleRule::setSelectorText() is currently not implemented but is beneficial at least for changing rule selectors via the Web Inspector.
Selector source range extraction is needed for the text-based CSS editing.

[Alexander Pavlov (apavlov)]

One thing I am unsure about is the ExceptionCodes I should return from setSelectorText for when there is no document associated with the CSSStyleRule being processed (which CSSParser needs to parse the selector) and there is an error parsing the selector. None of the w3c documents seem to cover this issue.

I have chosen the semantically similar WRONG_DOCUMENT_ERR and SYNTAX_ERR, but there may be another opinion.

[Alexander Pavlov (apavlov)]

Created attachment 68541 [details]
[PATCH] Suggested solution

[WebKit Review Bot]

Attachment 68541 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/css/CSSParser.cpp:5750:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Pavlov (apavlov)]

Created attachment 68544 [details]
[PATCH] Style fixed

[Anne van Kesteren]

Actually, per http://dev.w3.org/csswg/cssom/ you should not throw a SYNTAX_ERR (just ignore it). I'm not sure how you can get a CSSStyleRule separately from a style sheet.

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> Actually, per http://dev.w3.org/csswg/cssom/ you should not throw a SYNTAX_ERR (just ignore it). I'm not sure how you can get a CSSStyleRule separately from a style sheet.

You are correct. It's just I didn't connect this fact with the usage of CSSStyleRule::setSelectorText in JSCSSStyleRule.cpp. Anyway, if I implement it to always return "no error", it will be hard to tell if the operation has succeeded (e.g. due to a wrong selector list syntax).

[Alexander Pavlov (apavlov)]

Created attachment 68661 [details]
[PATCH] Do not return an ExceptionCode in the event of an error in setSelectorText()

[Alexander Pavlov (apavlov)]

Error handling fixed, I've got another question: is
doc->styleSelectorChanged(RecalcStyleImmediately);
the right way to notify the document of the rule selector change?

[Anne van Kesteren]

You can tell if setting succeeded by comparing the new string with the old string. A lot of web platform APIs taking strings work this way.

[Alexander Pavlov (apavlov)]

(In reply to comment #9)
> You can tell if setting succeeded by comparing the new string with the old string. A lot of web platform APIs taking strings work this way.

That's right - I didn't mean to say it was impossible (I said "hard" :)). But since I'm working directly with the CSSStyleRule native instance, I'd like to have a simple means of detecting the operation success/failure. I believe the API was dictated by the IDL and not intended to be interacted with from the native code.

On a related note, you must know that the selectorText setter previously used to raise DOMException, as per http://www.w3.org/TR/DOM-Level-2-Style/css.html#CSS-CSSStyleRule, not unreasonably, I should say :)

[Alexander Pavlov (apavlov)]

(In reply to comment #9)
> You can tell if setting succeeded by comparing the new string with the old string. A lot of web platform APIs taking strings work this way.

Ah, one more note. This approach will likely fail when setting the same selector (an idempotent operation), since the serialized selector list is canonicalized in the sense that extraneous whitespace is removed. Thus, "div span" (one space) and "div   span" (two spaces) correspond to the same selector, but cssRule.selectorText will return "div span" in both cases making one think that the operation has failed.

[Sam Weinig]

Comment on attachment 68661 [details]
[PATCH] Do not return an ExceptionCode in the event of an error in setSelectorText()

This should be broken into two at least two patches, one for the implementation of setSelectorText with tests, and one for the rest of the patch.

[Alexander Pavlov (apavlov)]

Created attachment 68918 [details]
[PATCH] Only source offsets extraction for rule selectors

[Alexander Pavlov (apavlov)]

Created attachment 69031 [details]
[PATCH] Fix selector list end detection

[Alexander Pavlov (apavlov)]

Created attachment 69794 [details]
[PATCH] Rebase + small API change

[Early Warning System Bot]

Attachment 69794 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4245052

[Eric Seidel (no email)]

Attachment 69794 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4150081

[Alexander Pavlov (apavlov)]

Created attachment 69804 [details]
[PATCH] Rebase for compilability

[Timothy Hatcher]

What is the UI for this?

[Pavel Feldman]

(In reply to comment #19)
> What is the UI for this?

There are no changes to the UI. The ultimate goal is to add support for style properties not supported by WebKit (moz- and such) without changing the style editing UI. We will also get persistence for local modifications and such. We are fairly close to switching to it!

[Pavel Feldman]

Comment on attachment 69804 [details]
[PATCH] Rebase for compilability

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

Overall looks good. Few nits from me and we are ready to land.

> WebCore/css/CSSParser.cpp:-152
> -    , m_currentStyleData(0)

Do not remove this line. You could access style data via rule data.

[Alexander Pavlov (apavlov)]

Created attachment 69938 [details]
[PATCH] Comment addressed

[Alexander Pavlov (apavlov)]

Committed: http://trac.webkit.org/changeset/69196