Bug 165831 - Web Inspector: Styles sidebar: Uncommenting CSS rules of pseudo-elements doesn't work
Summary: Web Inspector: Styles sidebar: Uncommenting CSS rules of pseudo-elements does...
Status: RESOLVED DUPLICATE of bug 166786
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: 166783
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-13 17:01 PST by Nikita Vasilyev
Modified: 2017-01-06 17:04 PST (History)
6 users (show)

See Also:


Attachments
[HTML] Reduction (247 bytes, text/html)
2016-12-13 17:01 PST, Nikita Vasilyev
no flags Details
[Animated GIF] Bug (412.48 KB, image/gif)
2016-12-13 17:04 PST, Nikita Vasilyev
no flags Details
[Animated GIF] Re-enabling disabled CSS rules is broken (118.17 KB, image/gif)
2016-12-19 14:43 PST, Nikita Vasilyev
no flags Details
Patch (3.47 KB, patch)
2016-12-21 16:58 PST, Nikita Vasilyev
mattbaker: review+
Details | Formatted Diff | Diff
Patch (3.39 KB, patch)
2016-12-22 13:20 PST, 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 2016-12-13 17:01:31 PST
Created attachment 297049 [details]
[HTML] Reduction

Steps:
1. Open attached index.html
2. Inspect .box:before
3. Click on the [P] icon in the styles sidebar to comment out all CSS properties of the selected rule
4. Click on the [P] icon again to uncomment all CSS properties

Expected:
CSS properties get uncommented.

Actual:
CSS properties don't get uncommented.
Comment 1 Nikita Vasilyev 2016-12-13 17:04:14 PST
Created attachment 297050 [details]
[Animated GIF] Bug
Comment 2 BJ Burg 2016-12-13 17:09:46 PST
Is this a regression?
Comment 3 Nikita Vasilyev 2016-12-13 17:15:31 PST
Yes, this works as expected in macOS Sierra Safari but not in ToT WebKit. I didn't bisect yet to find where exactly it regressed.
Comment 4 BJ Burg 2016-12-13 17:29:45 PST
<rdar://problem/29652688>
Comment 5 Nikita Vasilyev 2016-12-19 14:43:55 PST
Created attachment 297480 [details]
[Animated GIF] Re-enabling disabled CSS rules is broken

Looks like this isn't just about pseudo elements — it can happen for any element.
Comment 6 Nikita Vasilyev 2016-12-19 15:13:14 PST
(In reply to comment #5)
> Created attachment 297480 [details]
> [Animated GIF] Re-enabling disabled CSS rules is broken
> 
> Looks like this isn't just about pseudo elements — it can happen for any
> element.

[Error] Assertion Failed
	_associateRelatedProperties (DOMNodeStyles.js:934)
	_updateStyleCascade (DOMNodeStyles.js:824)
	fetchedInlineStyles (DOMNodeStyles.js:146)
	fetchedInlineStyles
	_dispatchResponseToCallback (InspectorBackend.js:311)
	_dispatchResponse (InspectorBackend.js:281)
	dispatch (InspectorBackend.js:157)
	dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:42)
Comment 7 Nikita Vasilyev 2016-12-20 16:02:56 PST
(In reply to comment #6)
> (In reply to comment #5)
> > Created attachment 297480 [details]
> > [Animated GIF] Re-enabling disabled CSS rules is broken
> > 
> > Looks like this isn't just about pseudo elements — it can happen for any
> > element.

This is reproducible in macOS Sierra as well.

> [Error] Assertion Failed
> 	_associateRelatedProperties (DOMNodeStyles.js:934)
> 	_updateStyleCascade (DOMNodeStyles.js:824)
> 	fetchedInlineStyles (DOMNodeStyles.js:146)
> 	fetchedInlineStyles
> 	_dispatchResponseToCallback (InspectorBackend.js:311)
> 	_dispatchResponse (InspectorBackend.js:281)
> 	dispatch (InspectorBackend.js:157)
> 	dispatchNextQueuedMessageFromBackend (MessageDispatcher.js:42)

This error appears to be unrelated.
Comment 8 Nikita Vasilyev 2016-12-20 17:16:04 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Created attachment 297480 [details]
> > > [Animated GIF] Re-enabling disabled CSS rules is broken
> > > 
> > > Looks like this isn't just about pseudo elements — it can happen for any
> > > element.
> 
> This is reproducible in macOS Sierra as well.

Uncommenting inline rules turned out to be a separate issue from uncommenting pseudo-element rules.
Bug 166297 - Web Inspector: Uncommenting CSS properties doesn't work for inline styles
Comment 9 Nikita Vasilyev 2016-12-21 16:58:08 PST
Created attachment 297633 [details]
Patch
Comment 10 Nikita Vasilyev 2016-12-21 17:07:42 PST
It appears this never worked. The regression was in: Bug 166297 - Web Inspector: Uncommenting CSS properties doesn't work for inline styles.
Comment 11 Matt Baker 2016-12-22 09:49:48 PST
Comment on attachment 297633 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:314
> +            for (var property of properties)

Nit: use `let`.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1294
> +        var textMarker = property.__propertyTextMarker;

Ditto.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1301
> +        var range = textMarker.find();

Ditto.
Comment 12 BJ Burg 2016-12-22 10:46:57 PST
(In reply to comment #10)
> It appears this never worked. The regression was in: Bug 166297 - Web
> Inspector: Uncommenting CSS properties doesn't work for inline styles.

I don't understand your comment. A regression means the functionality has been decreased. If it never worked, then there was nothing to have regressed.
Comment 13 Nikita Vasilyev 2016-12-22 13:12:19 PST
(In reply to comment #12)
> (In reply to comment #10)
> > It appears this never worked. The regression was in: Bug 166297 - Web
> > Inspector: Uncommenting CSS properties doesn't work for inline styles.
> 
> I don't understand your comment. A regression means the functionality has
> been decreased. If it never worked, then there was nothing to have regressed.

Uncommenting CSS rules of pseudo-elements by clicking [P] icon never worked.

The other bug was a regression: Bug 166297 - Web Inspector: Uncommenting CSS properties doesn't work for inline styles.
Comment 14 Nikita Vasilyev 2016-12-22 13:20:48 PST
Created attachment 297667 [details]
Patch
Comment 15 WebKit Commit Bot 2016-12-22 13:57:34 PST
Comment on attachment 297667 [details]
Patch

Clearing flags on attachment: 297667

Committed r210110: <http://trac.webkit.org/changeset/210110>
Comment 16 WebKit Commit Bot 2016-12-22 13:57:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Joseph Pecoraro 2017-01-05 16:49:20 PST
Comment on attachment 297667 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1290
>      _propertyCommentCheckboxChanged(event)
>      {
> -        var commentTextMarker = event.target.__commentTextMarker;
> -        console.assert(commentTextMarker);
> -        if (!commentTextMarker)
> +        this._uncommentProperty(event.target);
> +    }

Here event.target is not a "property" it is a "checkbox". It will never have __propertyTextMarker. It will however have __commentTextMarker. This code is very ambiguous. I'll look at cleaning this up.
Comment 18 Nikita Vasilyev 2017-01-05 17:56:26 PST
(In reply to comment #17)
> Comment on attachment 297667 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297667&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1290
> >      _propertyCommentCheckboxChanged(event)
> >      {
> > -        var commentTextMarker = event.target.__commentTextMarker;
> > -        console.assert(commentTextMarker);
> > -        if (!commentTextMarker)
> > +        this._uncommentProperty(event.target);
> > +    }
> 
> Here event.target is not a "property" it is a "checkbox". It will never have
> __propertyTextMarker. It will however have __commentTextMarker. This code is
> very ambiguous. I'll look at cleaning this up.

My change broke uncommenting properties by clicking the checkbox :(

Interestingly, commenting and uncommenting properties by pressing Cmd+/ still works, as it doesn't call _uncommentRange method.
Comment 19 Joseph Pecoraro 2017-01-06 16:14:24 PST
I'm going to roll this out. It causes new regressions and doesn't seem to properly solve the original problem (it fails if there are any shorthand properties in the rule). I'll be addressing all commenting / uncommenting issues that appeared with the new CSS parser in a separate bug. There are a few approaches we can take that fixes all of these issues.
Comment 20 WebKit Commit Bot 2017-01-06 16:18:10 PST
Re-opened since this is blocked by bug 166783
Comment 21 Joseph Pecoraro 2017-01-06 17:04:53 PST
Duping to the single bug to handle CSS Toggling issues:

*** This bug has been marked as a duplicate of bug 166786 ***