Last Comment Bug 740543 - Rule view does not update when window is resized
: Rule view does not update when window is resized
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: x86 Mac OS X
P2 normal (vote)
: Firefox 15
Assigned To: Dave Camp (:dcamp)
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
Depends on: 757253
  Show dependency treegraph
Reported: 2012-03-29 12:20 PDT by Kevin Dangoor
Modified: 2012-06-02 17:03 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

WIP (11.01 KB, patch)
2012-05-30 13:46 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
v1 (20.13 KB, patch)
2012-06-01 16:29 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
v2 (22.51 KB, patch)
2012-06-02 10:46 PDT, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Splinter Review
v3 (22.65 KB, patch)
2012-06-02 13:52 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review

Description User image Kevin Dangoor 2012-03-29 12:20:48 PDT

1. Open this test case:
2. open the page inspector
3. make sure the content area is fairly narrow (so that the media query kicks in)
4. lock on the red box and look at the rule view values
5. expand the window
6. the red box expands, but the rule view still says "width: 200px" and lists the media query

expected result: the rule view updates to reflect the new styles

(note that the property view is quite possibly doing the same thing, I just didn't look at this time)
Comment 1 User image Dave Camp (:dcamp) 2012-05-30 13:46:06 PDT
Created attachment 628457 [details] [diff] [review]

Doesn't have a good test yet, and I want to revisit one or two of the choices.
Comment 2 User image Dave Camp (:dcamp) 2012-06-01 16:29:44 PDT
Created attachment 629391 [details] [diff] [review]

I'm not a huge fan of this patch, but it makes the responsive design view much more useful.
Comment 3 User image Paul Rouget [:paul] 2012-06-02 06:19:02 PDT
Comment on attachment 629391 [details] [diff] [review]

Review of attachment 629391 [details] [diff] [review]:

So the inspector listens to the "resize" event of the page, then, emit a "resize" event. The ruleview listen to this event, and call a specific function (onResize).
I am not against this path, but this feels a lot "resize" specific.

What about that:
The inspector emits an event: "page_layout_update" on resize. Then the ruleview call scheduleRefresh when it gets this event.
In the future, we would use this event when we detect a layout update (on DOM updates, or if the JS code plays with the CSSOM).
And we could use this event in the layoutview as well.

(basically, just rename the event, and remove the onResize function).

Some other ideas:
Maybe it's better to listen the the MozAfterPaint events instead of "resize" (this is what I do for the layoutview).
And maybe it's better to do the "delaying" (250ms timer) on the inspector side.

What do you think?

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ +119,5 @@
>    this.inspector.on("change", this._onChange);
> +  this.inspector.on("sidebaractivated-ruleview", this._onChange);
> +
> +  this._onResize = this.onResize.bind(this);

Comment 4 User image Dave Camp (:dcamp) 2012-06-02 10:46:06 PDT
Created attachment 629497 [details] [diff] [review]

This version moves the timeout management into Inspector.  It uses a change event with a 'layout' detail.

As discussed on irc, for now we're only responding to resize events.
Comment 5 User image Dave Camp (:dcamp) 2012-06-02 10:47:03 PDT
Comment 6 User image Rob Campbell [:rc] (:robcee) 2012-06-02 13:40:04 PDT
Comment on attachment 629497 [details] [diff] [review]

+  _scheduleLayoutChange: function Inspector_scheduleLayoutChange()
+  {
+    if (this._timer) {
+      return null;
+    }
+    this._timer = Cc[";1"].createInstance(Ci.nsITimer);
+    this._timer.init(
+      function() {
+        this.change("layout");
+      }.bind(this),
+  },

Is this any different than just using setTimeout, other than looking cool?

13:25 < robcee> also, I'm not sure if we've gotten away from using isFunctionName for testing 
                functions that return booleans or not in inspector code
13:26 < robcee> panelVisible vs. isPanelVisible?

13:27 < robcee> yeah, I think we do
13:28 < robcee> isInspectorOpen, isDirty

13:30 < robcee> extra indents in your _thaw: function
13:31 < robcee> in _emit, there's a for each in loop you can switch to for .. of since you're in 
13:31 <@dcamp> oh right

(in CssRuleView)

+  get editing() {
+    return this.element.querySelectorAll(".styleinspector-propertyeditor").length > 0;
+  },


r+ with those.
Comment 7 User image Dave Camp (:dcamp) 2012-06-02 13:52:48 PDT
Created attachment 629518 [details] [diff] [review]

Fixed everything mentioned above, and added a few method comments because I want to be popular.
Comment 8 User image Dave Camp (:dcamp) 2012-06-02 17:03:39 PDT

Note You need to log in before you can comment on or make changes to this bug.