Closed Bug 1301078 Opened 3 years ago Closed 3 years ago

pasting a commented-out property does not work

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox51 verified)

VERIFIED FIXED
Firefox 51
Iteration:
51.3 - Sep 19
Tracking Status
firefox51 --- verified

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [reserve-html])

Attachments

(1 file)

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.
Iteration: --- → 51.3 - Sep 12
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [reserve-html]
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?
Attachment #8788960 - Flags: review?(pbrosset)
Blocks: 1301692
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?
Attachment #8788960 - Flags: review?(pbrosset) → review+
Flags: qe-verify? → qe-verify+
QA Contact: petruta.rasa
(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.
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.
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b1cb8b0be7c
correctly disable pasted commented-out properties; r=pbro
https://hg.mozilla.org/mozilla-central/rev/7b1cb8b0be7c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.