629002 - Arrow panels have rounding issues

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Try the URL field with a high zoom value: sometimes the arrow of the panel will not show (when you try to submit an invalid form) because the values are rounded too often and not in a consistent way.

I tried to not round the values but it happens while testing to see a panel 0.000001-ish inside the anchor. I can assume this is not a precision we want so rounding when all calculations are done should be correct.

Comment 1

[Neil Deakin]

Attachment: patch with different rounding

This patch is ok, but I found that when updating the test for bug 628238, it only worked if I used 'ceil' for the left and top coordinates and 'floor' for the right and bottom coordinates. (That is, the boxes are always rounded inwards)

There's always been some rounding issues with popups but doing this instead seems reasonable to me.

Mounir, could you test that (with the patch in bug 628238) with your cases?

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to comment #1)
> Created attachment 507837 [details] [diff] [review]
> patch with different rounding
> 
> This patch is ok, but I found that when updating the test for bug 628238, it
> only worked if I used 'ceil' for the left and top coordinates and 'floor' for
> the right and bottom coordinates. (That is, the boxes are always rounded
> inwards)
> 
> There's always been some rounding issues with popups but doing this instead
> seems reasonable to me.
> 
> Mounir, could you test that (with the patch in bug 628238) with your cases?

At a first glance, I don't understand how that could work better to ceil/floor instead of simple rounding. I re-checked my cases and I got some failure (but nothing dramatic and I'm going to investigate those) and the test in bug 628238 passes.

What was exactly failing for you?

Comment 3

[Mounir Lamouri (:mounir)]

I have only one issue which is not related to the rounding issue. With the GTK arrow panels, when the arrow panels should be placed on the top of a checkbox, I got weird values:
 popupTop 711
 popupBottom: 772
 anchorTop: 754
 anchorBottom: 768
But the popup is placed on top of the anchor.

I don't know if that's only for GTK arrow panels though...

Comment 4

[Mounir Lamouri (:mounir)]

Gasp, there is one situation where rounding wasn't a good idea but I thought it might be super rare... When popupBottom = 42.5 and anchorTop = 42.49, rounded it is changed to, respectively, 43 and 42.

You might have found a situation like that, that's why floor and ceil solved it. Unfortunately, it moves the problem elsewhere (42.99 and 43.00 would give 42 and 43 with floor). I guess we should just compare the raw values with an epsilon.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch with epsilon comparison

I think this is better than rounding. Now, the only issue I have is related to the checkbox. I've seen another one when the popup shows on top but has position like if it was on bottom so the anchor is on top of the popup (pointing nowhere).

Comment 6

[Neil Deakin]

Comment on attachment 508242 [details] [diff] [review]
Patch with epsilon comparison

>+        function smallerTo(aFloat1, aFloat2, aEpsilon)
>+        {
>+          if (aFloat1 < aFloat2) {
>+            return true;
>+          }
>+          return Math.abs(aFloat1 - aFloat2) <= aEpsilon;
>+        }

The function could just be: return aFloat1 <= aFloat2 + aEpsilon

>+        let epsilon = 0.1;

Use const here instead of let.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Patch with epsilon comparison

Comment 8

[Neil Deakin]

Comment on attachment 508934 [details] [diff] [review]
Patch with epsilon comparison

You have part of another patch or some debugging info in here. But the arrow panel changes are ok.

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 508934 [details] [diff] [review]
Patch with epsilon comparison

Indeed, the first chunck of this patch shouldn't be there.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 508934 [details] [diff] [review]
Patch with epsilon comparison

Please be sure to give a summary of risks vs. rewards of taking a patch when requesting review.

From what I can gather from bug comments the patch looks fairly involved and the behaviour it is addressing doesn't really look broken so I'm inclined to defer on it at this point.

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #10)
> Comment on attachment 508934 [details] [diff] [review]
> Patch with epsilon comparison
> 
> Please be sure to give a summary of risks vs. rewards of taking a patch when
> requesting review.
> 
> From what I can gather from bug comments the patch looks fairly involved and
> the behaviour it is addressing doesn't really look broken so I'm inclined to
> defer on it at this point.

The risks here seems smaller than the rewards: the arrow panel code is using the position of the anchor and the panel to know how to position the arrow. Because of rounding issues, it's currently wrong sometimes (very often when zooming). With this patch, it should be correct (mostly?) everytime.
Note that the issue described in comment 5 isn't related to this patch (I should actually open another bug for it).

Comment 12

[Dave Townsend [:mossop]]

So it seems to be right in the general cases and even when it is wrong it doesn't look bad. The risks seem to be that you're changing the positioning calculations which could regress positioning in some cases and you haven't added any tests to verify new the behaviour. That puts the weight firmly on the risk side in my book and we really need to ship.

Re-request approval if I've misread the situation otherwise this can land after we branch.

Comment 13

[Neil Deakin]

The tests are in bug 628238 which depend on this being fixed.

Comment 14

[Mounir Lamouri (:mounir)]

Comment on attachment 508934 [details] [diff] [review]
Patch with epsilon comparison

Re-asking approval per comment 13.

And I don't know how this could make the positioning worse than it already is. The current rounding is definitely wrong.

Comment 15

[Mounir Lamouri (:mounir)]

See comment 13 (and 14).

Comment 16

[Johnathan Nightingale [:johnath]]

Comment on attachment 508934 [details] [diff] [review]
Patch with epsilon comparison

We should take this fix, but not in the endgame of the release, imo. I think Dave's sentiment is right - clearly helps the general case, but makes me woozy to change the calculations right now.

Comment 17

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/48a752f312fc

Comment 18

[Neil Deakin]

Bah, I wanted to check this in with bugs 524545 and 628238 to prevent failures.

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to comment #18)
> Bah, I wanted to check this in with bugs 524545 and 628238 to prevent failures.

I didn't know, sorry :(