Bug 46367 - CSSParser: Enable rule selector source range extraction
Summary: CSSParser: Enable rule selector source range extraction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks: 45825
  Show dependency treegraph
 
Reported: 2010-09-23 10:08 PDT by Alexander Pavlov (apavlov)
Modified: 2010-10-06 08:46 PDT (History)
16 users (show)

See Also:


Attachments
[PATCH] Suggested solution (12.08 KB, patch)
2010-09-23 10:31 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Style fixed (12.04 KB, patch)
2010-09-23 10:37 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Do not return an ExceptionCode in the event of an error in setSelectorText() (11.97 KB, patch)
2010-09-24 01:56 PDT, Alexander Pavlov (apavlov)
sam: review-
Details | Formatted Diff | Diff
[PATCH] Only source offsets extraction for rule selectors (10.30 KB, patch)
2010-09-27 09:19 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Fix selector list end detection (10.27 KB, patch)
2010-09-28 01:29 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Rebase + small API change (18.96 KB, patch)
2010-10-05 09:07 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Rebase for compilability (18.96 KB, patch)
2010-10-05 10:14 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Comment addressed (22.08 KB, patch)
2010-10-06 07:31 PDT, Alexander Pavlov (apavlov)
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2010-09-23 10:08:52 PDT
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.
Comment 1 Alexander Pavlov (apavlov) 2010-09-23 10:29:25 PDT
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.
Comment 2 Alexander Pavlov (apavlov) 2010-09-23 10:31:41 PDT
Created attachment 68541 [details]
[PATCH] Suggested solution
Comment 3 WebKit Review Bot 2010-09-23 10:33:39 PDT
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.
Comment 4 Alexander Pavlov (apavlov) 2010-09-23 10:37:22 PDT
Created attachment 68544 [details]
[PATCH] Style fixed
Comment 5 Anne van Kesteren 2010-09-23 23:33:43 PDT
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.
Comment 6 Alexander Pavlov (apavlov) 2010-09-24 01:16:59 PDT
(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).
Comment 7 Alexander Pavlov (apavlov) 2010-09-24 01:56:46 PDT
Created attachment 68661 [details]
[PATCH] Do not return an ExceptionCode in the event of an error in setSelectorText()
Comment 8 Alexander Pavlov (apavlov) 2010-09-24 02:10:14 PDT
Error handling fixed, I've got another question: is
doc->styleSelectorChanged(RecalcStyleImmediately);
the right way to notify the document of the rule selector change?
Comment 9 Anne van Kesteren 2010-09-24 03:01:51 PDT
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.
Comment 10 Alexander Pavlov (apavlov) 2010-09-24 03:26:36 PDT
(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 :)
Comment 11 Alexander Pavlov (apavlov) 2010-09-24 03:31:59 PDT
(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.
Comment 12 Sam Weinig 2010-09-24 10:52:24 PDT
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.
Comment 13 Alexander Pavlov (apavlov) 2010-09-27 09:19:04 PDT
Created attachment 68918 [details]
[PATCH] Only source offsets extraction for rule selectors
Comment 14 Alexander Pavlov (apavlov) 2010-09-28 01:29:27 PDT
Created attachment 69031 [details]
[PATCH] Fix selector list end detection
Comment 15 Alexander Pavlov (apavlov) 2010-10-05 09:07:43 PDT
Created attachment 69794 [details]
[PATCH] Rebase + small API change
Comment 16 Early Warning System Bot 2010-10-05 09:17:13 PDT
Attachment 69794 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4245052
Comment 17 Eric Seidel (no email) 2010-10-05 09:27:04 PDT
Attachment 69794 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4150081
Comment 18 Alexander Pavlov (apavlov) 2010-10-05 10:14:25 PDT
Created attachment 69804 [details]
[PATCH] Rebase for compilability
Comment 19 Timothy Hatcher 2010-10-05 10:17:27 PDT
What is the UI for this?
Comment 20 Pavel Feldman 2010-10-05 10:37:21 PDT
(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!
Comment 21 Pavel Feldman 2010-10-06 05:42:16 PDT
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.
Comment 22 Alexander Pavlov (apavlov) 2010-10-06 07:31:49 PDT
Created attachment 69938 [details]
[PATCH] Comment addressed
Comment 23 Alexander Pavlov (apavlov) 2010-10-06 08:46:00 PDT
Committed: http://trac.webkit.org/changeset/69196