Closed Bug 807571 Opened 8 years ago Closed 8 years ago

popupboxobject's sizeTo() will not resize if already open and new size specifies the same height

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: markh, Assigned: markh)

Details

Attachments

(1 file, 1 obsolete file)

STR:
* Have a panel open with a size of (x,y)
* While it is open, call panel.sizeTo(z,y)
Expected:
* Panel adjusts to the new width
Actual:
* Panel doesn't resize until it is next shown.

Problem is nsPopupBoxObject::SizeTo() calls:

  content->SetAttr(kNameSpaceID_None, nsGkAtoms::width, width, false);
  content->SetAttr(kNameSpaceID_None, nsGkAtoms::height, height, true);

so only the setting of the height passes true for 'aNotify' - however, content->SetAttr() notices the attribute value is the same as it was previously and doesn't notify the listeners, so no layout happens.
OS: Windows 7 → All
Hardware: x86_64 → All
Under the assumption we don't want to pass aNotify=true twice, this patch checks the current attribute values and only sets them if different while still arranging to set aNotify for the final change.  Includes a test.
Attachment #677654 - Flags: feedback?(enndeakin)
Comment on attachment 677654 [details] [diff] [review]
check attribute values before setting them

>+
>+  // We only want to pass aNotify=true to SetAttr once, but must make sure
>+  // we pass it when a value is being changed - which forces us to get the
>+  // current attributes and check if they are being changed.
>+  bool widthSame = content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::width, width, eCaseMatters);
>+  bool heightSame = content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::height, height, eCaseMatters);
>+
>+  if (!widthSame)
>+    content->SetAttr(kNameSpaceID_None, nsGkAtoms::width, width, heightSame);
>+  if (!heightSame)
>+    content->SetAttr(kNameSpaceID_None, nsGkAtoms::height, height, true);

Since SetAttr already checks if the values are the same, you don't need to check if the width is the same. You should be able to just do:

content->SetAttr(kNameSpaceID_None, nsGkAtoms::width, width, heightSame);
content->SetAttr(kNameSpaceID_None, nsGkAtoms::height, height, true);

w h
0 0 true true
0 1 false true
1 0 true true
1 1 false true

>+      // resize to 10px bigger in both dimensions.
>+      sizeAndCheck(bcr.width+10, bcr.height+10);
>+      // Same width, different height.
>+      sizeAndCheck(bcr.width+10, bcr.height);
>+      // Same height, different width
>+      sizeAndCheck(bcr.width, bcr.height);
>+      event.target.hidePopup();

The comments don't seem to match what you're doing on each line. There should also be a test where the width is unchanged and the height is changed.
Attachment #677654 - Flags: feedback?(enndeakin) → feedback+
(In reply to Neil Deakin from comment #2)
> Comment on attachment 677654 [details] [diff] [review]
> check attribute values before setting them
> Since SetAttr already checks if the values are the same, you don't need to
> check if the width is the same. You should be able to just do:

Done.

> The comments don't seem to match what you're doing on each line. There
> should also be a test where the width is unchanged and the height is changed.

I think they did match and does test that case, but it wasn't clear from the code.  I hope this next version makes it clearer.
Attached patch As described.Splinter Review
Assignee: nobody → mhammond
Attachment #677654 - Attachment is obsolete: true
Attachment #677949 - Flags: review?(enndeakin)
The test was better before. In the original test, I was referring to this:

+      // Same width, different height.
+      sizeAndCheck(bcr.width+10, bcr.height);

The comment says 'same width' but you change the width and don't change the height.

+      // Same height, different width
+      sizeAndCheck(bcr.width, bcr.height);

Here you don't change either yet the comment says 'different width'.

And you should add a line that changes the height, but doesn't change the width.
(In reply to Neil Deakin from comment #5)
> The test was better before. In the original test, I was referring to this:
> 
> +      // Same width, different height.
> +      sizeAndCheck(bcr.width+10, bcr.height);
> 
> The comment says 'same width' but you change the width and don't change the
> height.

The full snippet is:

+      var bcr = panel.getBoundingClientRect();
+      // resize to 10px bigger in both dimensions.
+      sizeAndCheck(bcr.width+10, bcr.height+10);
+      // Same width, different height.
+      sizeAndCheck(bcr.width+10, bcr.height);
+      // Same height, different width
+      sizeAndCheck(bcr.width, bcr.height);

Note the first call resizes to |width+10|, |height+10|.  

The second call is *still* using |width+10| - so it is the same width - and |height| - so a different height than the previous height+10.

The last call is back to |width| - smaller than the previous call, and |height| - the same as the previous call.

So assuming the result of getBoundingClientRect isn't "live", I think the comments do reflect the code.
Comment on attachment 677949 [details] [diff] [review]
As described.

Ah, of course. So use the C++ changes from the new patch and the old version of the test. Perhaps make the comments clearer so that its obvious the you're relying on the new width/height each time.
Attachment #677949 - Flags: review?(enndeakin) → review+
Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/59e5f407eb75
Flags: in-testsuite+
Version: 16 Branch → Trunk
https://hg.mozilla.org/mozilla-central/rev/59e5f407eb75
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.