1194851 - [RTL] [APZ] Fake-scrollbar-space is drawn in scrollable things when APZ enabled

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect, P1)

Description

[Alice0775 White]

Attachment: screencast showing fake-scrollbar-space

+++ This bug was initially created as a clone of Bug #1194346 +++


Steps To Reproduce
1. Start RTL language Firefox(Hebrew version) and Enable APZ(default in Nightly42.0a1).
2. Create about:config pref "dom.allow_XUL_XBL_for_file", set to true, so that you can load XUL directly.
3. Download & load attachment 8648184 [details] testcase.
4. (optional) Reduce the width of your Firefox window.
5. (optional) reload or scroll the listbox.

Actual Results:
 Fake-scrollbar-space is drawn at the right side of the listbox

Expected Results:
 No such fake-scrollbar-space

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Alice0775 White from comment #0)
> 1. Start RTL language Firefox(Hebrew version) and Enable APZ(default in
> Nightly42.0a1).

I can reproduce this in a standard Firefox Nightly version (doesn't have to be Hebrew).

(And note that on Linux nightlies, APZ is not yet on by the default - you have to turn on pref "layers.async-pan-zoom.enabled" and restart Firefox. With that, I can reproduce this bug.)

Comment 2

[Daniel Holbert [:dholbert]]

I can actually reproduce this with a <textarea>, too -- no need for XUL. Testcase coming up, & reclassifying.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1 (with textarea)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screenshot (APZ enabled on top, disabled on bottom)

Here's a screenshot comparing an APZ-enabled profile (top) to an APZ-disabled profile [default on linux], (bottom), viewing the testcase I just attached.

Note the difference in the right edge of the textboxes.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

This is not a clipping issue as I originally suspected; it's that the displayport is truncated. The displayport computation involves taking the displayport base rect, doing a bunch of stuff to expand it, and then clamping it to the scrollable rect size. The base rect has an x>0 because of the scrollbar being on the left. The scrollable rect has an x=0. And then then two are intersected, resulting in the truncated displayport width.

Fundamentally it's a coordinate space mismatch but I'm not sure which coordinate space is the "correct" one to be working in here. Having a base rect with x=0 would in theory give the correct displayport result but it might be more conceptually correct to shift the scrollable rect and un-shift the displayport after the intersection.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Actually there's other stuff I should work on first.

Comment 8

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1194851 - Don't clip scrolled contents with display ports if the scrollport doesn't start at (0,0). r?kats

Bug 1194851 - Don't clip scrolled contents with display ports if the scrollport doesn't start at (0,0). r?kats

Comment 10

[Markus Stange [:mstange]]

I'm not really sure whether that's the right thing to do, or whether the comment is correct. It seems to work as expected, though. For example I'm not suddenly clipping away stuff in the top left corner.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8655687 [details]
MozReview Request: Bug 1194851 - Don't clip scrolled contents with display ports if the scrollport doesn't start at (0,0). r?kats

I'm probably not the right person to review this; tn knows this code much better. Redirecting review to him.

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8655687 [details]
MozReview Request: Bug 1194851 - Don't clip scrolled contents with display ports if the scrollport doesn't start at (0,0). r?kats

https://reviewboard.mozilla.org/r/17985/#review16141

Good catch.

I feel like this mistake is probably being made in several other places in our code, we should audit.

Comment 13

[Timothy Nikkel (:tnikkel)]

Hmm, actually the displayport base is the original dirty rect, so it will be relative to mOuter as well. And we just expand that by the display port margins. So shouldn't the display port be relative to mOuter then as well?

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8655687 [details]
MozReview Request: Bug 1194851 - Don't clip scrolled contents with display ports if the scrollport doesn't start at (0,0). r?kats

https://reviewboard.mozilla.org/r/17985/#review16143

Comment 15

[Timothy Nikkel (:tnikkel)]

Looks like the problem is in GetDisplayPortFromMarginsData http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=359809ae3db3#1004 when we intersect the display port with the expanded scrollable rect. Up to that point the base rect and hence the displayport is relative to the scrollframe, but the expanded scrollable rect is relative to the scrollport.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

And I take it the adjustment by scrollPos we do on the next line is insufficient to compensate for this? What other component are we missing there?

Comment 18

[magicp]

No longer responding. How about?

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 2 (with listbox from <select size="3">)

I can reproduce this bug with an HTML listbox (<select size="...">).  Specifically, when I view the attached testcase in Nightly with a fresh profile, the 3rd digit is missing in each line of the RTL listbox.

(I can fix this by disabling e10s in Firefox Preferences, or by disabling layers.async-pan-zoom.enabled.)

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Copying over regression properties from bug 1193781 comment 2.

Comment 22

[Botond Ballo [:botond]]

I'm going to look at this while Timothy is away.

Comment 23

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1194851 - Add helper functions HasDisplayPort() and HasCriticalDisplayPort(). r=kats

Review commit: https://reviewboard.mozilla.org/r/28209/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28209/

Comment 24

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1194851 - Remove the second parameter of IsFixedPosFrameInDisplayPort(). r=kats

Review commit: https://reviewboard.mozilla.org/r/28211/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28211/

Comment 25

[Botond Ballo [:botond]]

^ These are some preliminary patches that make it easier to audit the places in the code that actually consume the value of the displayport.

Comment 26

[Botond Ballo [:botond]]

Based on my conversation with Kats today and some investigation, here's my summary of the problem:

  - Depending on where it's used, the displayport is interpreted as being relative to
    either the scroll frame, or the scroll port.

  - The scroll port's position relative to the scroll frame is frequently (0, 0)
    (which is why this has gone unnoticed for so long), but it can be nonzero if
    there are borders involved or there's a non-overlay scrollbar in an RTL setup.

Auditing the places that actually consume the value of the displayport:

  (a) nsLayoutUtils::ComputeFrameMetrics         [1]
  (b) ScrollFrameHelper::DecideScrollableLayer   [2]
  (c) ScrollFrameHelper::IsRectNearlyVisible     [3]
  (d) ScrollFrameHelper::ScrollToImpl            [4] [5]
  (e) nsDisplaySubDocument::ComputeVisibility    [6]
  (f) PresShell::MarkImagesInSubtreeVisible      [7]
  (g) ContainerState::ComputeOpaqueRect          [8]

[1] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/base/nsLayoutUtils.cpp#8441
[2] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/generic/nsGfxScrollFrame.cpp#3204
[3] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/generic/nsGfxScrollFrame.cpp#3344
[4] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/generic/nsGfxScrollFrame.cpp#2524
[5] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/generic/nsGfxScrollFrame.cpp#2540
[6] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/base/nsDisplayList.cpp#4500
[7] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/base/nsPresShell.cpp#5677
[8] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/base/FrameLayerBuilder.cpp#3736

  (a) Ships the displayport off to the compositor, which interpets it as being
      relative to the scroll port.

  (b), (c), (e), (f), and (g) interpret the displayport as being relative to
      the scroll frame (as far as I can tell).

  (d) Doesn't care because it just cares about whether the displayport has changed.

So this is what the displayport is _expected_ to be relative to.

As Timothy points out in comment 15, what the value of the displayport is _actually_ relative to is something nonsensical, because the value is arrived at in GetDisplayPortFromMarginsData() by intersecting a rectangle relative to the scroll port (the expanded scrollable rect) with a rectangle relative to the scroll frame (the display port base inflated by the margins).

Given this, here's my proposed fix approach:

  - Fix GetDisplayPortFromMarginsData() by translating the expanded scrollable rect
    to be relative to the scroll frame (by adding to it the position of the scroll
    port relative to the scroll frame) prior to the intersection. This ensures that
    the resulting rect is properly relative to the scroll frame.

  - As most consumers either expect the displayport to be relative to the scroll
    frame or don't care, this means most consumers don't need to be changed.

  - The one consumer that expects the displayport to be relative to the scroll port,
    ComputeFrameMetrics(), can translate it back to be thus.

Thoughts?

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8699247 [details]
MozReview Request: Bug 1194851 - Add helper functions HasDisplayPort() and HasCriticalDisplayPort(). r=kats

https://reviewboard.mozilla.org/r/28209/#review25295

Nice cleanup, thanks!

::: layout/base/nsLayoutUtils.cpp:1082
(Diff revision 1)
> +nsLayoutUtils::HasDisplayPort(nsIContent* aContent) {

nit: brace on newline

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8699248 [details]
MozReview Request: Bug 1194851 - Remove the second parameter of IsFixedPosFrameInDisplayPort(). r=kats

https://reviewboard.mozilla.org/r/28211/#review25297

Comment 29

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #26)
> Based on my conversation with Kats today and some investigation, here's my
> summary of the problem:

Thanks for the detailed analysis! Documenting this here makes it really easy to follow :)

> Given this, here's my proposed fix approach:
> 
>   - Fix GetDisplayPortFromMarginsData() by translating the expanded
> scrollable rect
>     to be relative to the scroll frame (by adding to it the position of the
> scroll
>     port relative to the scroll frame) prior to the intersection. This
> ensures that
>     the resulting rect is properly relative to the scroll frame.
> 
>   - As most consumers either expect the displayport to be relative to the
> scroll
>     frame or don't care, this means most consumers don't need to be changed.
> 
>   - The one consumer that expects the displayport to be relative to the
> scroll port,
>     ComputeFrameMetrics(), can translate it back to be thus.
> 
> Thoughts?

Assuming the conversion back and forth is lossless, this sounds fine to me. My impression from talking to Timothy was that he was going to make the displayport getter take an extra parameter that would return it in one of the two spaces, but that adds more complexity so I prefer this approach.

Comment 30

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #26)
>   (a) Ships the displayport off to the compositor, which interpets it as
> being
>       relative to the scroll port.

Ah, this is what I needed to know. I looked at some of the uses of displayport on the frame metrics, and they were relative to the scrollport, but didn't get to look at them all.

I was planning to finish this off before I came here to see the progress, since it looks like you are on pto/holidays I'll steal this back, unless there are objections.

Comment 31

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #30)
> I was planning to finish this off before I came here to see the progress,
> since it looks like you are on pto/holidays I'll steal this back, unless
> there are objections.

No objection - thanks!

Comment 32

[Timothy Nikkel (:tnikkel)]

Two new things I've found:
-the coordinate system of the display port base isn't consistent. APZC callback helper sets them to the composition bounds (ie relative to the scroll port), layout does relative to scrollframe.
-the uses of displayport might be heavily weighted towards wanting them relative to the scrollport, but the uses of the nsLayoutUtils::GetDisplayPort itself mostly want it relative to the scroll frame. I think the count was 7 (scrollframe) to 2 (scrollport). So in the interest of not repeating the adjustment code everywhere having a getter GetDisplayPortRelativeToScrollFrame I think is a good idea. This gets more complicated because we have GetOrMaybeCreateDisplayPort, ViewportHasDisplayPort that also want the displayport relative the scrollframe, but who's name might make it hard to make this distinction clear.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66ae33d18362
https://hg.mozilla.org/integration/mozilla-inbound/rev/764b4012f30b

Comment 34

[Botond Ballo [:botond]]

I landed the small refactoring patches posted in comment 23 and comment 24. Marking bug as leave-open so that the bug doesn't get closed when these merge to m-c.

Timothy is actively working on the actual fix, based on the plan described in comment 32; updating assignee to reflect this.

Comment 35

[Timothy Nikkel (:tnikkel)]

Attachment: Part 3. Change one instance of GetDisplayPort to HasDisplayPort

Comment 36

[Timothy Nikkel (:tnikkel)]

Attachment: Part 4. Make ViewportHasDisplayPort only return a bool

Comment 37

[Timothy Nikkel (:tnikkel)]

Attachment: Part 5. Simplify GetOrMaybeCreateDisplayPort into MaybeCreateDisplayPort

Feel free to pass any review onto Markus if you would prefer.

Comment 38

[Timothy Nikkel (:tnikkel)]

Attachment: Part 6. Store the displayport base relative to the scrollport.

Comment 39

[Timothy Nikkel (:tnikkel)]

Attachment: Part 7. Create GetDisplayPortRelativeToScrollFrame

Comment 40

[Timothy Nikkel (:tnikkel)]

Attachment: Part 8. Temporarily rename GetDisplayPort to GetDisplayPortRelativeToScrollPort and convert all GetDisplayPort callers.

Comment 41

[Timothy Nikkel (:tnikkel)]

Attachment: Part 9. Rename GetDisplayPortRelativeToScrollPort back to GetDisplayPort

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66ae33d18362
https://hg.mozilla.org/mozilla-central/rev/764b4012f30b

Comment 43

[Timothy Nikkel (:tnikkel)]

Attachment: Part 7. Create GetDisplayPortRelativeToScrollFrame

Comment 44

[Timothy Nikkel (:tnikkel)]

Attachment: Part 8. Change GetDisplayPortForVisibilityTesting

Comment 45

[Timothy Nikkel (:tnikkel)]

Attachment: Part 9. Temporarily rename GetDisplayPort to GetDisplayPortRelativeToScrollPort and convert all GetDisplayPort callers.

Comment 46

[Timothy Nikkel (:tnikkel)]

Attachment: Part 10. Rename GetDisplayPortRelativeToScrollPort back to GetDisplayPort

Comment 47

[Botond Ballo [:botond]]

Comment on attachment 8704057 [details] [diff] [review]
Part 4. Make ViewportHasDisplayPort only return a bool

Review of attachment 8704057 [details] [diff] [review]:
-----------------------------------------------------------------

I guess both in this patch and the next one, we're making a tradeoff of having an extra GetProperty() call (one when checking if the displayport exists, and the second when getting it) to get a simpler interface?

::: layout/base/nsLayoutUtils.h
@@ +526,5 @@
>                                          bool aIsClipFixed);
>  
>    /**
>     * Return true if aPresContext's viewport has a displayport.
>     * Fills in aDisplayPort with the displayport rectangle if non-null.

Update comment

Comment 48

[Botond Ballo [:botond]]

Comment on attachment 8704061 [details] [diff] [review]
Part 6. Store the displayport base relative to the scrollport.

Review of attachment 8704061 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3235,5 @@
> +    } else {
> +      // Restrict the dirty rect to the scrollport, and make it relative to the
> +      // scrollport for the displayport base.
> +      displayportBase = aDirtyRect->Intersect(mScrollPort);
> +      displayportBase -= mScrollPort.TopLeft();

All added lines should be intended by one more level

Comment 49

[Botond Ballo [:botond]]

Comment on attachment 8704584 [details] [diff] [review]
Part 7. Create GetDisplayPortRelativeToScrollFrame

Review of attachment 8704584 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +850,5 @@
>    return ApplyRectMultiplier(aRectData->mRect, aMultiplier);
>  }
>  
> +nsIFrame*
> +GetScrollFrameFromContent(nsIContent* aContent)

Can we refactor nsLayoutUtils::FindScrollableFrameFor to use this?

@@ +1085,5 @@
>    return GetDisplayPortImpl(aContent, aResult, 1.0f);
>  }
>  
>  bool
> +nsLayoutUtils::GetDisplayPortRelativeToScrollFrame(nsIContent* aContent, nsRect *aResult)

nsRect* aResult

Comment 50

[Botond Ballo [:botond]]

Comment on attachment 8704585 [details] [diff] [review]
Part 8. Change GetDisplayPortForVisibilityTesting

Review of attachment 8704585 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.h
@@ +184,5 @@
>    /**
>     * @return the display port for the given element which should be used for
>     * visibility testing purposes.
>     *
>     * If low-precision buffers are enabled, this is the critical display port;

"the critical display port relative to the scroll frame"

Comment 51

[Botond Ballo [:botond]]

Comment on attachment 8704587 [details] [diff] [review]
Part 9. Temporarily rename GetDisplayPort to GetDisplayPortRelativeToScrollPort and convert all GetDisplayPort callers.

Review of attachment 8704587 [details] [diff] [review]:
-----------------------------------------------------------------

One possible way the final API could look is:

  enum class RelativeTo {
    ScrollPort,
    ScrollFrame
  };

  bool GetDisplayPort(nsIContent* aContent, nsRect* aResult, RelativeTo aRelativeTo = RelativeTo::ScrollPort) {
    // get it relative to the scroll port
    if (aRelativeTo == RelativeTo::ScrollFrame) {
      // translate it
    }
  }

I'm fine with the API these patches put in place, too; just throwing out a possible alternative idea.

Comment 52

[Botond Ballo [:botond]]

Thanks for breaking up this patch set so nicely! It made it easy to review.

Comment 53

[Timothy Nikkel (:tnikkel)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bcda7e04d00

Comment 54

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #51)
> One possible way the final API could look is:

That was actually my first idea, but I think kats said it seemed more complicated, but he might have been meaning something else. So that's why I went a different direction.

I can add a patch on the end of these to change the api to this if we all agree.

Comment 55

[Kartikaya Gupta (email:[e-mail])]

I liked botond's proposal in comment 26 but I haven't looked at the patches/code in detail so if something else works out better that's fine. I'm happy to leave it to you guys.

Comment 56

[Botond Ballo [:botond]]

The difference between my proposal in comment 26 and what the current patch implements is:

  - In my proposal, there would have been just one API (GetDisplayPort()) which 
    returns the display port relative to the scroll frame, and call sites that 
    want it relative to the scroll port would adjust it themselves.

  - With the current patches, there are two APIs, GetDisplayPort() which returns
    the display port relative to the scroll port, and GetDisplayPortRelativeToScrollFrame()
    which returns the display port relative to the scroll frame.

In retrospect, I think Timothy's approach is actually better, because internally GetDisplayPort() gets the display port relative to the scroll port, so in my approach it would have had to adjust it, and then callers that wanted it relative to the scroll port would have had to un-adjust it. In other words, Timothy's approach avoids the "conversion back and forth" mentioned in comment 29.

My suggestion in comment 51 is just a cosmetic one, where, rather than having the two APIs look like this:

   GetDisplayPort(content, rect)
   GetDisplayPortRelativeToScrollFrame(content, rect)

they would look like this:

   GetDisplayPort(content, rect)
   GetDisplayPort(content, rect, RelativeTo::ScrollFrame)

(with the first one being a shorthand, using a default argument, for

   GetDisplayPort(content, rect, RelativeTo::ScrollPort)

). Timothy, it's up to you whether you'd like to make this cosmetic change or not; you have my r+ either way.

Comment 57

[Timothy Nikkel (:tnikkel)]

I like the enum api. The only part I don't like is that we need to type nsLayoutUtils::RelativeTo everywhere, which makes it a little longer.

Comment 58

[Timothy Nikkel (:tnikkel)]

Attachment: Part 11. Change api to optional RelativeTo parameter.

Comment 59

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #57)
> I like the enum api. The only part I don't like is that we need to type
> nsLayoutUtils::RelativeTo everywhere, which makes it a little longer.

We could put RelativeTo outside of nsLayoutUtils.

Comment 60

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #47)
> I guess both in this patch and the next one, we're making a tradeoff of
> having an extra GetProperty() call (one when checking if the displayport
> exists, and the second when getting it) to get a simpler interface?

Yeah, but we GetProperty should be one hashtable lookup, and we do tons and tons of those for every paint, so a drop in the bucket.

Comment 61

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #59)
> We could put RelativeTo outside of nsLayoutUtils.

Okay, I'll do that. Was worried about polluting the namespace. But I guess it's fine.

Comment 62

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #61)
> (In reply to Botond Ballo [:botond] from comment #59)
> > We could put RelativeTo outside of nsLayoutUtils.
> 
> Okay, I'll do that. Was worried about polluting the namespace. But I guess
> it's fine.

We could even reuse it in the future for things that aren't the displayport and can be relative to various things. (Although then we may need to check for an unsupported value of RelativeTo being passed to a function like GetDisplayPort).

Comment 63

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #49)
> >  bool
> > +nsLayoutUtils::GetDisplayPortRelativeToScrollFrame(nsIContent* aContent, nsRect *aResult)
> 
> nsRect* aResult

This already had inconsistent style on GetDisplayPort, so I think I'll just leave the style fixes because I kill this function in the last patch of the series and I don't want to do a ton of rebasing for it.

Comment 64

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #49)
> ::: layout/base/nsLayoutUtils.cpp
> > +nsIFrame*
> > +GetScrollFrameFromContent(nsIContent* aContent)
> 
> Can we refactor nsLayoutUtils::FindScrollableFrameFor to use this?

Good catch. I'll do this in another bug because I have another patch that changes this function that will go in yet another bug.

Comment 65

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #64)
> (In reply to Botond Ballo [:botond] from comment #49)
> > ::: layout/base/nsLayoutUtils.cpp
> > > +nsIFrame*
> > > +GetScrollFrameFromContent(nsIContent* aContent)
> > 
> > Can we refactor nsLayoutUtils::FindScrollableFrameFor to use this?
> 
> Good catch. I'll do this in another bug because I have another patch that
> changes this function that will go in yet another bug.

Bug 1237813.

Comment 66

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0455bbd5b471
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d96f186754
https://hg.mozilla.org/integration/mozilla-inbound/rev/66a487d737ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ceeb943d908
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d51f59e534
https://hg.mozilla.org/integration/mozilla-inbound/rev/e419bcecce50
https://hg.mozilla.org/integration/mozilla-inbound/rev/19982832e209
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1221ced178e
https://hg.mozilla.org/integration/mozilla-inbound/rev/909f6cca63b7

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0455bbd5b471
https://hg.mozilla.org/mozilla-central/rev/c3d96f186754
https://hg.mozilla.org/mozilla-central/rev/66a487d737ed
https://hg.mozilla.org/mozilla-central/rev/2ceeb943d908
https://hg.mozilla.org/mozilla-central/rev/e0d51f59e534
https://hg.mozilla.org/mozilla-central/rev/e419bcecce50
https://hg.mozilla.org/mozilla-central/rev/19982832e209
https://hg.mozilla.org/mozilla-central/rev/e1221ced178e
https://hg.mozilla.org/mozilla-central/rev/909f6cca63b7

Comment 68

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Reproduced with all attached test cases.

Verified as fixed on Firefox 46 beta 9.