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

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: check attribute values before setting them

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.

Comment 2

[Neil Deakin]

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.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: As described.

Comment 5

[Neil Deakin]

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.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

(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 7

[Neil Deakin]

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.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/59e5f407eb75

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/59e5f407eb75