Closed Bug 1224504 Opened 4 years ago Closed 4 years ago

[devtools] Editing in Box Model view does not get applied immediately to Rules view

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox44 affected, firefox45 affected, firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox44 --- affected
firefox45 --- affected
firefox50 --- verified

People

(Reporter: magicp.jp, Assigned: nchevobbe)

References

Details

(Whiteboard: [btpp-fix-later])

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151112030238

Steps to reproduce:

1. Run Nightly (or Aurora)
2. Open any sites (e.g. addons.mozilla.org)
3. Open devtools > Inspector > Box Model
4. Select any elements in HTML pane
5. Edit padding, margin and border values in Box Model view
6. Switch to Rules view


Actual results:

Editing in Box Model view does not applied immediately to Rules view when switching tab. (need to select other elements and go back)
In the opposite navigation, editing in Rules view is applied immediately to Box Model view.


Expected results:

Editing in Box Model view should be applied immediately to Rules view when switching tab.
Has STR: --- → yes
Component: Untriaged → Developer Tools: Inspector
OS: Unspecified → All
Hardware: Unspecified → All
Matteo, we've discussed about something similar when you were working on the geometryEditor (the rule-view would not update live). I don't remember what we decided: did you end up fixing it when working on the geometryEditor, or did we decide to leave it for a follow-up (in which case, if you had filed a bug, then it is a dupe of this one I guess).
Flags: needinfo?(zer0)
Priority: -- → P2
Whiteboard: [btpp-fix-later]
Assignee: nobody → chevobbe.nicolas
This might be a duplicate of bug 1264950. Some investigation needs to happen first to make sure they're really the same bug.
See Also: → 1264950
Duplicate of this bug: 1264950
That's what the code intended to do, but it was checking an unexisting property.
This patch fixes that and adds a test to make sure changes are synced in the rule
view when editing properties in the layout view.

Review commit: https://reviewboard.mozilla.org/r/59518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59518/
Attachment #8763250 - Flags: review?(pbrosset)
Comment on attachment 8763250 [details]
Bug 1224504 - Selecting the rule view panel should refresh the panel content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59518/diff/1-2/
Attachment #8763250 - Flags: review?(pbrosset) → review?(jdescottes)
https://reviewboard.mozilla.org/r/59518/#review56716

Looks good to me, thanks Nicolas!
 
Only one suggestion: the ruleview uses _viewedElement to point to the inspected element, but the computed view uses viewedElement (no underscore). For consistency, it could be nice to rename viewedElement to _viewedElement in inspector/computed/computed.js and in inspector/computed/test/browser_computed_matched-selectors_01.js

::: devtools/client/inspector/layout/test/browser_layout_sync.js:41
(Diff revision 2)
> +
> +  info("Wait for the rule view to be refreshed");
> +  yield onRuleViewRefreshed;
> +  ok(true, "The rule view was refreshed");
> +
> +  let ruleEditor = ruleView.element.children[0]._ruleEditor;

We could move the method getRuleViewRuleEditor defined in inspector/rules/test/head.js to inspector/test/head.js and use it here. Note that a duplicated method can be found in inspector/shared/test/head.js and could also be removed.
Comment on attachment 8763250 [details]
Bug 1224504 - Selecting the rule view panel should refresh the panel content.

https://reviewboard.mozilla.org/r/59518/#review56718
Attachment #8763250 - Flags: review?(jdescottes) → review+
Summary: [devtools] Editing in Box Model view does not applied immediately to Rules view → [devtools] Editing in Box Model view does not get applied immediately to Rules view
Comment on attachment 8763250 [details]
Bug 1224504 - Selecting the rule view panel should refresh the panel content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59518/diff/2-3/
Comment on attachment 8763631 [details]
Bug 1224504 - Use the same variable name in CssComputedView than in CssRuleView for inspected element.

https://reviewboard.mozilla.org/r/59804/#review56758

Thanks, looks good to me !
Attachment #8763631 - Flags: review?(jdescottes) → review+
Comment on attachment 8763250 [details]
Bug 1224504 - Selecting the rule view panel should refresh the panel content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59518/diff/3-4/
Comment on attachment 8763631 [details]
Bug 1224504 - Use the same variable name in CssComputedView than in CssRuleView for inspected element.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59804/diff/1-2/
Even if there are failures in the TRY run (https://treeherder.mozilla.org/#/jobs?repo=try&revision=02c5d73227b6&selectedJob=22599063), I don't see how any of them should be a consequence of my patches (there's only one dt- failure, https://treeherder.mozilla.org/#/jobs?repo=try&revision=02c5d73227b6&selectedJob=22597889, and it's a test on a different folder than my patches).
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/3b8ab4fefde1
Selecting the rule view panel should refresh the panel content. r=jdescottes
https://hg.mozilla.org/integration/fx-team/rev/21a0421ca9b0
Use the same variable name in CssComputedView than in CssRuleView for inspected element. r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3b8ab4fefde1
https://hg.mozilla.org/mozilla-central/rev/21a0421ca9b0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Tested on Windows 7 on latest Nightly 50.0a1 (2016-06-23) and the properties are added to the element style as expected.
As this got fixed now, I'm clearing the ni for Matteo.

Sebastian
Status: RESOLVED → VERIFIED
Flags: needinfo?(zer0)
No longer blocks: top-inspector-bugs
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.