Tom Tromey :tromey Assignee
	Description • 8 years ago

While investigating a test failure with bug 1265796, I found a
pre-existing rule view bug (I think a regression from as-authored
but I didn't try to verify that).

The failure came from
devtools/client/inspector/rules/test/browser_rules_add-property-commented.js

STR:

Open the rule view
Start editing to make a new property
Paste a commented-out property like "/* background: yellow; */"
Now notice that while the rule view says it is disabled, it actually isn't;
and in the style editor the new property is not commented out.

Clicking on the button in the rule view to enable the property will give
an exception.

	Patrick Brosset <:pbro>
	Comment 2 • 8 years ago
mozreview-review

Comment on attachment 8788960 [details]
Bug 1301078 - correctly disable pasted commented-out properties;

https://reviewboard.mozilla.org/r/77268/#review76272

::: devtools/client/inspector/rules/models/rule.js:191
(Diff revision 1)
> -    this.applyProperties((modifications) => {
> +    let addPromise = this.applyProperties((modifications) => {
>        modifications.createProperty(ind, name, value, priority);
> +    });
> +
> +    // If not enabled, do a second edit to disable the property.
> +    if (!enabled) {
> +      addPromise = this.applyProperties((modifications) => {
> +        modifications.setPropertyEnabled(ind, name, false);
> +      });
> +    }

Would it be possible to avoid creating the property in the first place? 
Here we're creating it, then disabling it, and then updating the editor.
And in fact, you can see in the page that the background changes quickly and then reverts.

Or maybe we do need to create it so the cssText on the actor is changed, but maybe we could pass a boolean to createProperty that says the property should be disabled.

Let me know if that would work?

	Patrick Brosset <:pbro>
	Comment 4 • 8 years ago
mozreview-review

Comment on attachment 8788960 [details]
Bug 1301078 - correctly disable pasted commented-out properties;

https://reviewboard.mozilla.org/r/77268/#review76302

This looks really good.
Just double checking, since we're using the as-authored impl, we don't have to worry about backward compat with adding an extra argument, right?

	Tom Tromey :tromey Assignee
	Comment 5 • 8 years ago

(In reply to Patrick Brosset <:pbro> from comment #4)
> Comment on attachment 8788960 [details]
> Bug 1301078 - correctly disable pasted commented-out properties;
> 
> https://reviewboard.mozilla.org/r/77268/#review76302
> 
> This looks really good.
> Just double checking, since we're using the as-authored impl, we don't have
> to worry about backward compat with adding an extra argument, right?

Yes, that's correct.
In the as-authored case we send the entire rule text as a string to the server.
In the non-authored case, disabled properties are simply never sent.

	Tom Tromey :tromey Assignee
	Comment 6 • 8 years ago

With this change, it turns out that calls to RuleEditor.addProperty:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/views/rule-editor.js#363

... now should pass enabled=true to avoid adding a disabled rule.
This affected some of the tests, which call this directly with just 3 arguments.
I'll attach a new patch shortly.

	Tom Tromey :tromey Assignee
	Comment 8 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63ccd1a1547f531312ac70ed1b4ee461a58aca45

	Pulsebot
	Comment 9 • 8 years ago

Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b1cb8b0be7c
correctly disable pasted commented-out properties; r=pbro

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7b1cb8b0be7c

	Petruta Horea [:phorea], Desktop QA
	Comment 11 • 8 years ago

Reproduced on Nightly 51.0a1 2016-09-12.

Verified as fixed using 51.0a1 Nightly build from 2016-09-13 under Win 10 64-bit, Mac OS X 10.11 and Ubuntu 14.04 64-bit.