In rule view, the 'computed' (shorthand expansion) properties aren't working when a new rule is added

RESOLVED FIXED in Firefox 41

Status

()

Firefox
Developer Tools: Inspector
P1
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: bgrins, Assigned: jsnajdr)

Tracking

unspecified
Firefox 41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8599554 [details]
expanded-property.png

STR:

Open inspector
Add a new rule: `padding: 4px 0`
Click the expansion arrow

Expected to see this: 

padding-top: 4px;
padding-right: 0px;
padding-bottom: 4px;
padding-left: 4px;

Actually seeing this: 

padding-top: ;
padding-right: ;
padding-bottom: ;
padding-left: ;
Priority: -- → P1
Assignee: nobody → zer0
(Assignee)

Comment 1

3 years ago
Related: when an existing rule is edited, then the expanded view is not updated - still shows the old values.
(Assignee)

Comment 2

3 years ago
Created attachment 8609326 [details] [diff] [review]
In rule view, update the computed values of a property after editing

This bug happens not only when a new rule is added, but every time a property value is edited. Adding a new rule is just a special case: in that scenario, a new property is added first and then an editor is launched on the empty value.

Why it happens?
1. While you are editing a property value, method Rule.previewPropertyValue is called to preview the effect of the new value in real-time. In this method, the value of the TextProperty is changed, which is wrong. Only the 'modifications' should be created and applied.

2. When Rule.setPropertyValue is called later, the property already has a desired value and the method is a noop. This is wrong, because Rule.applyProperties is not called for this change. Therefore, TextProperty.updateComputed is not called either and the computed list has wrong (old) values.

Solution: don't set the property value in previewPropertyValue.

Side note: The 'aName' parameter in Rule.applyProperties seems to be obsolete - it's not used. Can I remove it? Or am I missing something?

Asking for feedback. I'm going to write a new test case for this now and submit an updated patch when I'm finished.
Attachment #8609326 - Flags: feedback?(zer0)
(Assignee)

Comment 3

3 years ago
Created attachment 8609877 [details] [diff] [review]
In rule view, update the computed values of a property after editing

Added a test case for this bug. It has several issues though and I need some advice:

1. The test needs to give TextPropertyEditor._previewValue a chance to be called. The call is throttled to be delayed by 10ms. So I added a 100ms wait after entering a new value and before doing anything else. Such a wait is of course not very nice. A better solution would be to wait for the style in the page content to be updated. But I don't know how to do it. What event to wait for? Is there another mochitest that already does something similar?

2. The test passes when run on a fixed build, but on an unfixed one, it timeouts waiting for a 'ruleview-changed' event, instead of failing quickly. That's because in a buggy version, the event is never fired. Is this really a problem or can it be ignored? Any idea how to fix it?

3. I created a separate file for the test: browser_ruleview_edit-property-commit.js. Maybe it should be merged into some existing file?
Attachment #8609877 - Flags: feedback?(zer0)
(Assignee)

Updated

3 years ago
Attachment #8609326 - Attachment is obsolete: true
Attachment #8609326 - Flags: feedback?(zer0)
Assignee: zer0 → jsnajdr
Status: NEW → ASSIGNED
(In reply to Jarda Snajdr [:jsnajdr] from comment #3)
Drive-by quick feedback/review.
> Added a test case for this bug. It has several issues though and I need some
> advice:
Thanks for adding the test!
> 1. The test needs to give TextPropertyEditor._previewValue a chance to be
> called. The call is throttled to be delayed by 10ms. So I added a 100ms wait
> after entering a new value and before doing anything else. Such a wait is of
> course not very nice. A better solution would be to wait for the style in
> the page content to be updated. But I don't know how to do it. What event to
> wait for? Is there another mochitest that already does something similar?
Using timeouts in tests is definitely a bad idea. For information, when our tests run on build machines (especially on TRY), they run really slowly compared to your typical desktop/laptop, so 100ms might not always be enough. So, listening for an event is a better solution. I think there's a few mochitests that do this already.
Something like this should work:
let onStyleUpdated = waitForComputedStyleProperty("#testid", null, "padding", "20px");
> 2. The test passes when run on a fixed build, but on an unfixed one, it
> timeouts waiting for a 'ruleview-changed' event, instead of failing quickly.
> That's because in a buggy version, the event is never fired. Is this really
> a problem or can it be ignored? Any idea how to fix it?
It's not much of a problem I think. If you have no other way for the test to quickly fail if this part of the code is buggy, then timeout is fine too.
> 3. I created a separate file for the test:
> browser_ruleview_edit-property-commit.js. Maybe it should be merged into
> some existing file?
Creating new test files is the way to go if we want to keep short, simple to read, tests, so no problem there.
Comment on attachment 8609877 [details] [diff] [review]
In rule view, update the computed values of a property after editing

Review of attachment 8609877 [details] [diff] [review]:
-----------------------------------------------------------------

Your reasoning about how to get this fix seems correct, I haven't looked at it into too much details yet, but here is some feedback for the test.

::: browser/devtools/styleinspector/test/browser_ruleview_edit-property-computed.js
@@ +9,5 @@
> +add_task(function*() {
> +  yield addTab("data:text/html;charset=utf-8,test updating computed values of a style after value edit");
> +
> +  info("Creating the test document");
> +  createDocument();

The createDocument function manipulates content.document directly, which means it manipulates cross-process objects when the tests are run in e10s mode (e10s is the multi-process version of firefox, which will be the default in a very short while). Even if they work, we're trying to get rid of using cross-process objects in tests, so you should remove createDocument and instead:
- either create a new HTML file that contains the html and css you want, like a lot of other tests do,
- or put the html and css code in a string here, encodeURI it and put it in the addTab data-uri instead of "test updating computed values...."

@@ +51,5 @@
> +  info("Focusing the inplace editor field");
> +  let editor = yield focusEditableField(propEditor.valueSpan);
> +  is(inplaceEditor(propEditor.valueSpan), editor, "Focused editor should be the value span.");
> +
> +  let onModifications = idRuleEditor.rule._applyingModifications;

Please remove this line, you're not using onModifications later anyway, and we're trying to get rid of using _applyingModifications in our tests anyway. See bug 1166774.
Attachment #8609877 - Flags: feedback?(zer0) → feedback+
(Assignee)

Comment 6

3 years ago
Created attachment 8614079 [details] [diff] [review]
In rule view, update the computed values of a property after editing

Improved the test according to feedback:
- encode the testing document into the URL - it's too short to have its own HTML file
- removed waiting for timeout, waiting for computed property in the document to change instead
Attachment #8614079 - Flags: review?(pbrosset)
(Assignee)

Updated

3 years ago
Attachment #8609877 - Attachment is obsolete: true

Updated

3 years ago
Blocks: 1167669
Comment on attachment 8614079 [details] [diff] [review]
In rule view, update the computed values of a property after editing

Review of attachment 8614079 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/styleinspector/test/browser_ruleview_edit-property-computed.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +// Test that the computed values of a style (the shorthand expansion) are properly
> +// updated after the style is changed. See bug 1159922.

I don't think we need to worry about referencing the bug number as long as it is clear what the test is doing in the description.

@@ +9,5 @@
> +add_task(function*() {
> +  let testDocument = '<html>'
> +    + '<head><style type="text/css">#testid { padding: 10px; }</style></head>'
> +    + '<body><div id="testid">Styled Node</div></body>'
> +    + '</html>';

You can format the test document similar to what you see here: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/test/browser_ruleview_search-filter_01.js?from=test/browser_ruleview_search-filter_01.js&case=true#11

It is not necessary to include <html>, <head>, and <body>

@@ +11,5 @@
> +    + '<head><style type="text/css">#testid { padding: 10px; }</style></head>'
> +    + '<body><div id="testid">Styled Node</div></body>'
> +    + '</html>';
> +
> +  info("Creating the tab with test document");

I don't think we need these info dumps anymore since it is pretty clear what is being performed from the function name, and the info does not provide any additional information.

@@ +14,5 @@
> +
> +  info("Creating the tab with test document");
> +  yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(testDocument));
> +
> +  info("Opening the rule view");

Same as above

@@ +17,5 @@
> +
> +  info("Opening the rule view");
> +  let {toolbox, inspector, view} = yield openRuleView();
> +
> +  info("Selecting the test node");

Same as above
(Assignee)

Comment 8

3 years ago
Created attachment 8614711 [details] [diff] [review]
In rule view, update the computed values of a property after editing

Fixed things reported by reviewer:
- nicer formatting of the testing document
- removed useless info messages, left the ones in editAndCheck function, as they describe what the test does
Attachment #8614711 - Flags: review?(gabriel.luong)
Comment on attachment 8614079 [details] [diff] [review]
In rule view, update the computed values of a property after editing

As discussed, marking this one as obsolete because bzexport didn't.
Attachment #8614079 - Attachment is obsolete: true
Attachment #8614079 - Flags: review?(pbrosset)
Comment on attachment 8614711 [details] [diff] [review]
In rule view, update the computed values of a property after editing

Stealing review from Gabriel.
Attachment #8614711 - Flags: review?(gabriel.luong) → review?(pbrosset)
Comment on attachment 8614711 [details] [diff] [review]
In rule view, update the computed values of a property after editing

Review of attachment 8614711 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Here's a try build to check that nothing bad happened with this fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=000f47e2c728
Let's keep an eye on this, and ask for check-in when all test jobs turn green.

::: browser/devtools/styleinspector/test/browser_ruleview_edit-property-computed.js
@@ +1,4 @@
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +

I don't have any remark about this test, it looks nice and does the job.
For info, we recently added eslint to the devtools code as a way to get a more consistent code style.
It's only a manual thing for now, so if you have it set up locally, good, otherwise you won't get any warnings for the new code you're writing.
Here's more info about it: https://wiki.mozilla.org/DevTools/CodingStandards
I can only suggest you to install it in your code editor.
For example, on this file, eslint complains about a few lines being more than 80 chars long. Try to split them in a way that they remain readable.

@@ +16,5 @@
> +    '<div id="testid">Styled Node</div>'
> +  ].join('\n');
> +
> +  yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
> +  let {toolbox, inspector, view} = yield openRuleView();

toolbox isn't used, you can remove it here.
Attachment #8614711 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #11)
> Here's a try build to check that nothing bad happened with this fix:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=000f47e2c728
This shows that the patch causes a permanent test failure:
> 4763 INFO TEST-UNEXPECTED-FAIL | browser/devtools/styleinspector/test/browser_ruleview_cubicbezier-revert-on-ESC.js | Test timed out - expected PASS
Could you investigate why this test fails with your changes?
(Assignee)

Comment 13

3 years ago
The bezier tooltip logic seems to be broken.
(Assignee)

Comment 14

3 years ago
It turns out that my patch is not so great as it breaks the following:
- cancelling edit by ESC (both in text value editor and in swatches) doesn't revert the styles used in document - it reverts only the data in the rules inspector
- when editing a value using a swatch (color/bezier/filter), the value is not updated in real-time in the property text view

I will need to find a better solution.
(Assignee)

Comment 15

3 years ago
Created attachment 8615959 [details] [diff] [review]
In rule view, update the computed values of a property after editing

OK, here is a new patch that seems to work nice and passes all tests in the styleinspector suite.

I reviewed and cleaned up the logic for the property value edit cycle: change the value, preview the new value in the content document, and then either commit or cancel. Both the rule editor model and the content document styles are properly updated in all cases. Works with in-place text editor and all the swatches (color/bezier/filter).

This is how editing works now:
- when typing into the TextPropertyEditor's in-place editor, or when with playing with swatches, the underlying model (TextProperty) value is NOT UPDATED. It was sometimes updated before this patch and it led to buggy behavior.
- the only thing I do during the edit is to call previewPropertyValue in order to preview the change in real-time in the content document.
- only after committing the new value is applied to the TextProperty model object, triggering all associated actions: recompute the computed properties, mark the property as dirty by adding it to store.userProperties etc.
- when the edit is cancelled by ESC, a proper cleanup is performed: this means updating the value in TextPropertyEditor (e.g., in the view), and undoing the preview in the content document (by calling Rule.previewPropertyValue again, with the old value)

I also did some related refactorings:
- unified methods _onValueDone and _applyNewValue into one method called _onValueDone. They both do the same thing. _applyNewValue was only used by the swatches and contained code that could never work (like referencing property this.prop.rule.name which never exists).
- removed aName parameter from Rule.applyProperties as it is not used. All this code I removed looks like some ancient remains from times before e10n.
- fixed some ESLint errors about unused variables and imports
- I didn't fix any ESLint complaints about lines longer than 80 chars, because I think breaking them would hurt readability.
Attachment #8615959 - Flags: review?(pbrosset)
(Assignee)

Updated

3 years ago
Attachment #8614711 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
There are a few more things in rule-view.js that should be refactored:
- TextPropertyEditor doesn't need to have 'this.committed' property. The committed value is always available in the model (TextProperty)
- the TextProperty.editing flag is checked at many places, but never set to true or false. Dead code.
- the code for adding event handlers to swatches (TextPropertyEditor.update) is duplicated three times.
Comment on attachment 8615959 [details] [diff] [review]
In rule view, update the computed values of a property after editing

Review of attachment 8615959 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for digging in this complex problem Jarda. Unfortunately, as you can see for yourself, the rule-view isn't the simplest thing in the world, it hasn't aged well and we haven't spent much time trying to clean things up.
Your approach sounds very good to me, I hope this isn't going to be break too many tests on TRY though, they all pass fine locally for me.
In any case you should be aware of bug 1166774 which is very close to landing and that changes many tests.

Anyway, as I said, the approach looks good, and the code changes style are fine. Also, thanks for the cleanup.
Attachment #8615959 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 18

3 years ago
Created attachment 8620912 [details] [diff] [review]
In rule view, update the computed values of a property after editing

Resolved conflicts with patch to bug 1158288 (ESLint changes)
Attachment #8620912 - Flags: review?(pbrosset)
(Assignee)

Updated

3 years ago
Attachment #8615959 - Attachment is obsolete: true
Comment on attachment 8620912 [details] [diff] [review]
In rule view, update the computed values of a property after editing

Review of attachment 8620912 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that looks good.
Also, try seems green enough to me, you can add the "checkin-needed" keyword.
Attachment #8620912 - Flags: review?(pbrosset) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ac0a2b395a31
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
QA Whiteboard: [good first verify]

Updated

a year ago
Depends on: 1327788
You need to log in before you can comment on or make changes to this bug.