Bug 165831 – Web Inspector: Styles sidebar: Uncommenting CSS rules of pseudo-elements doesn't work

• Component: Web Inspector
• Status: RESOLVED DUPLICATE

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 297050 [details]
[Animated GIF] Bug

[Blaze Burg]

Is this a regression?

[Nikita Vasilyev]

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.

[Blaze Burg]

<rdar://problem/29652688>

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

(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)

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

(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

[Nikita Vasilyev]

Created attachment 297633 [details]
Patch

[Nikita Vasilyev]

It appears this never worked. The regression was in: Bug 166297 - Web Inspector: Uncommenting CSS properties doesn't work for inline styles.

[Matt Baker]

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.

[Blaze Burg]

(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.

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

Created attachment 297667 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 297667 [details]
Patch

Clearing flags on attachment: 297667

Committed r210110: <http://trac.webkit.org/changeset/210110>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

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.

[Nikita Vasilyev]

(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.

[Joseph Pecoraro]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 166783

[Joseph Pecoraro]

Duping to the single bug to handle CSS Toggling issues:

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