1031188 - Ensure that accDescription never duplicates AccessibleName

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Spun off bug 670083.
If the obtained accessible description matches the calculated name, never expose it. Screen readers would otherwise speak the information twice when the element is focused, unless they filter duplication out themselves. But since not all screen readers do that, we should do the filtering.

Comment 1

[Marco Zehe (:MarcoZ)]

In Accessible::Description, add logic to make sure that the description obtained never matches the name for the accessible. It would be advantageous to fix bug 1031184 first, whose template is already in bug 670083.

Comment 2

[Eric Skoglund [:tephra]]

I would love to try this as my first bug. 
Would be able to do bug 1031184 to if that would make things
easier.

Comment 3

[Marco Zehe (:MarcoZ)]

Eric, feel free to do it with or without bug 1031184, it's up to your personal preference.

Comment 4

[Eric Skoglund [:tephra]]

(In reply to Marco Zehe (:MarcoZ) from comment #3)
> Eric, feel free to do it with or without bug 1031184, it's up to your
> personal preference.

Ok, I'll start with this one then.

Comment 5

[Eric Skoglund [:tephra]]

Attachment: rev1

Comment 6

[Marco Zehe (:MarcoZ)]

Comment on attachment 8447909 [details] [diff] [review]
rev1

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

::: accessible/generic/Accessible.cpp
@@ +281,5 @@
> +  
> +  if (aDescription.Equals(name)) {
> +    // Don't expose any description if it is the same as the name
> +    aDescription.Truncate();
> +  } else if (aDescription.IsEmpty()) {

Yes, this looks good for this go-round. This lays the basis for when the below code is moved out into its own method (e. g. bug 1031184), and another check needs to happen for each time the description is checked for when empty.

The only thing left now is add some tests to make sure this and the below code paths work. E. g. construct an aria-describedby block in the test file that results in the same description and make sure the resulting description is empty. You may even be able to adjust an existing test for this. Cancelling review for now. Please re-request review once you have added tests

Good work!

Comment 7

[Eric Skoglund [:tephra]]

Attachment: rev2

Comment 8

[Eric Skoglund [:tephra]]

Attachment: Testing hmtl

Added three test to cover the branches for getting a description

Comment 9

[Eric Skoglund [:tephra]]

Attachment: Testing XUL

Test file for XUL to cover the last branch for getting the accessible description. Never done any XUL before so hopefully it is correct.

Comment 10

[Marco Zehe (:MarcoZ)]

Comment on attachment 8448093 [details] [diff] [review]
rev2

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

Hmm, I am not really happy with the code repetitions that creep in here. I'll comment, but also ask our module owner for additional feedback to get a second opinion.

::: accessible/generic/Accessible.cpp
@@ +276,5 @@
>      GetTextEquivFromIDRefs(this, nsGkAtoms::aria_describedby,
>                             aDescription);
> +  
> +  nsAutoString name;
> +  ENameValueFlag nameFlag = Name(name);

I'd move this name getting to before the first description fetch. That will make it clearer that the name will be used in all subsequent operations, and not interrupt the flow of getting the description and compressing its whitespace (see below).

@@ +278,5 @@
> +  
> +  nsAutoString name;
> +  ENameValueFlag nameFlag = Name(name);
> +
> +  aDescription.CompressWhitespace();

I wonder if this should rather be done in nsTextEquivUtils::GetTextEquivFromIDRefs so it always returns a whitespace-compressed string. Alex, what do you think?

@@ +282,5 @@
> +  aDescription.CompressWhitespace();
> +  
> +  if (aDescription.Equals(name)) {
> +    // Don't expose any description if it is the same as the name.
> +    aDescription.Truncate();

You can also return here, since you already got a description. You would then also not need to make the below an else block. So better check for !aDescription.IsEmpty(), and if true, check if it equals the name and truncate, and in any case, return since processing of aria-describedby yielded a valid result and the method is done.

I think this method should really be restructured in a way that it returns as soon as it has a valid description and doesn't keep falling through to the end. This would mean moving some code around into less indented blocks, but I think it would make this method more readable. Would you agree?

Comment 11

[Marco Zehe (:MarcoZ)]

Comment on attachment 8448094 [details] [diff] [review]
Testing hmtl

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

r=me for these.

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 8448095 [details] [diff] [review]
Testing XUL

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

r=me with that one comment addressed/fixed.

::: accessible/tests/mochitest/test_descr_xul.xul
@@ +2,5 @@
> +<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
> +
> +<window
> +        id="findfile-window"
> +        label="test"

Does the window element in XUL really support the label attribute? If not, this should be removed since it is superflous. :) Please attach a new patch if this is to be removed, but you don't need to re-request review.

Comment 13

[Eric Skoglund [:tephra]]

> @@ +278,5 @@
> > +  
> > +  nsAutoString name;
> > +  ENameValueFlag nameFlag = Name(name);
> > +
> > +  aDescription.CompressWhitespace();
> 
> I wonder if this should rather be done in
> nsTextEquivUtils::GetTextEquivFromIDRefs so it always returns a
> whitespace-compressed string. Alex, what do you think?
>

According to http://www.w3.org/TR/html4/types.html#type-cdata, leading and trailing whitespace can safely be disregarded in label, title etc.

> @@ +282,5 @@
> > +  aDescription.CompressWhitespace();
> > +  
> > +  if (aDescription.Equals(name)) {
> > +    // Don't expose any description if it is the same as the name.
> > +    aDescription.Truncate();
> 
> You can also return here, since you already got a description. You would
> then also not need to make the below an else block. So better check for
> !aDescription.IsEmpty(), and if true, check if it equals the name and
> truncate, and in any case, return since processing of aria-describedby
> yielded a valid result and the method is done.
> 
> I think this method should really be restructured in a way that it returns
> as soon as it has a valid description and doesn't keep falling through to
> the end. This would mean moving some code around into less indented blocks,
> but I think it would make this method more readable. Would you agree?

Yes that would make it more readable, it would then look something like this for each
branch:

if (!aDescription.IsEmpty()) {
   // Check if it is the same as name and if true truncate and return
} else { 
   // Attempt to get a valid description
}

Still leaves the problem with code duplication though.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 8448093 [details] [diff] [review]
rev2

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

::: accessible/generic/Accessible.cpp
@@ +326,1 @@
>    aDescription.CompressWhitespace();

why we cannot do simple

Name(name);
if (aDescription.Equals(name))
  aDescription.Truncate();

Comment 15

[Eric Skoglund [:tephra]]

(In reply to alexander :surkov from comment #14)
> Comment on attachment 8448093 [details] [diff] [review]
> rev2
> 
> Review of attachment 8448093 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/Accessible.cpp
> @@ +326,1 @@
> >    aDescription.CompressWhitespace();
> 
> why we cannot do simple
> 
> Name(name);
> if (aDescription.Equals(name))
>   aDescription.Truncate();

Seemingly GetTextEquivFromIDRefs does not return a whitespace compressed string and it seems like it doesn't return the correct string. For example the description returned for this:
    
 <p id="description">aria description</p>
 <img id="img4" alt="aria description" aria-describedby="description">

Is "aria description " and not "aria description" which would be the correct one. If we have GetTextEquivFromIDRefs return a whitespace compressed string we would not have to do it in the description method.

Comment 16

[Eric Skoglund [:tephra]]

Attachment: XUL Testing

Removed the label property since it is not needed

Comment 17

[Eric Skoglund [:tephra]]

Attachment: bug1031188_xul_test.diff

Removed label from window element since it is not needed

Comment 18

[alexander :surkov (:asurkov)]

I don't follow GetTextEquivFromIDRefs problem, either way you reach end of Accessible::Description() where you do CompressWhitespace, if you compare calculated description with accessible name there then that's it, no?

Comment 19

[Eric Skoglund [:tephra]]

(In reply to alexander :surkov from comment #18)
> I don't follow GetTextEquivFromIDRefs problem, either way you reach end of
> Accessible::Description() where you do CompressWhitespace, if you compare
> calculated description with accessible name there then that's it, no?

Oh sorry, yes. 

GetTextEquivFromIDRefs returns a string with a space (" ") character at the end of it
and that is why the CompressWhitspace is necessary so that we can compare it with the name.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to eric from comment #19)
> GetTextEquivFromIDRefs returns a string with a space (" ") character at the
> end of it
> and that is why the CompressWhitspace is necessary so that we can compare it
> with the name.

I don't really suggest to remove final CompressWhitspace call

Comment 21

[Eric Skoglund [:tephra]]

(In reply to alexander :surkov from comment #20)
> (In reply to eric from comment #19)
> > GetTextEquivFromIDRefs returns a string with a space (" ") character at the
> > end of it
> > and that is why the CompressWhitspace is necessary so that we can compare it
> > with the name.
> 
> I don't really suggest to remove final CompressWhitspace call

If we per marcos review make the function return as soon as a valid description is found the last CompressWhitespace will not matter.

Comment 22

[Eric Skoglund [:tephra]]

Attachment: rev3

As per the review the method now returns as soon as a valid description is found. There is still some code duplication here but I could not find a clean way to not have it there. If the different branches are broken into their own methods it might look cleaner.

Comment 23

[Marco Zehe (:MarcoZ)]

Comment on attachment 8455039 [details] [diff] [review]
rev3

This looks good to me. Yes that bit of code duplication can't be avoided as long as it all sticks in this one method, but that's what the NativeDescription bug is for. :) So if you want to, you can take that one up right after this and clean stuff up there.

Comment 24

[Eric Skoglund [:tephra]]

Thanks!

Added checkin-needed keyword. I can take up the NativeDescription bug tomorrow. 
Will comment there.

Comment 25

[Carsten Book [:Tomcat]]

Hi Guys,

checkin needed requests need now a passed try run to avoid bustages etc, could you provide a link to a passed try run and request checkin needed then again ? Maybe also https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices is helpful

Comment 26

[Eric Skoglund [:tephra]]

(In reply to Carsten Book [:Tomcat] from comment #25)
> Hi Guys,
> 
> checkin needed requests need now a passed try run to avoid bustages etc,
> could you provide a link to a passed try run and request checkin needed then
> again ? Maybe also
> https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices is
> helpful

Sorry must have missed that step in the guide, will get right to it.

Comment 27

[Marco Zehe (:MarcoZ)]

Try run is at: https://tbpl.mozilla.org/?tree=Try&rev=50f3e20dbe28

Comment 28

[Marco Zehe (:MarcoZ)]

Sorry Eric, might have crossed. I just kicked off one for the latest patch (rev 3).

Comment 29

[Eric Skoglund [:tephra]]

(In reply to Marco Zehe (:MarcoZ) from comment #28)
> Sorry Eric, might have crossed. I just kicked off one for the latest patch
> (rev 3).

Oh good, hadn't got it going yet still reading the process :)

Thanks for the help.

Comment 30

[Marco Zehe (:MarcoZ)]

Looks like at least one failure occurs:

1933 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/name/test_svg.html | Wrong description for 'svg1' - got A description

Comment 31

[Eric Skoglund [:tephra]]

Hmm, I'm quite certain  that I ran all the tests sorry about that.

I looked into this quickly and it seems like a " " character is appended somewhere (presumably this is what the last CompressWhitespace call was for). The description returned is "A description " but the expected description is "A description". I would think that AppendTextEquivFromContent is not returning the correct string.

As I see it there are two solutions:
 
    1. Make sure that AppendTextEquivFromContent returns a whitespace compressed string 
      (same for GetTextEquivFromIDRefs)

    2. Add CompressWhitespace after the calls causing the behavior (Seems like the messier 
       and worser option)

Comment 32

[Marco Zehe (:MarcoZ)]

I'd prefer option 1, too.

Comment 33

[Eric Skoglund [:tephra]]

I can start working on that later this evening.

Comment 34

[Eric Skoglund [:tephra]]

Sorry for the silence have been in Greece for a week. Working on making AppendTextEquivFromContent return a whitespace compressed string still.

Comment 35

[Eric Skoglund [:tephra]]

Attachment: bug1031188_rev4.diff

I do apologize for the longer time on this one. I wasn't able to change the GetTextEquivFromIdRefs to return a whitespace compressed string. I did a compromise for now, I took out the different parts of Description and made them into two new methods (as by bug 1031184), these methods returns a whitespacecompressed description. Ran all tests in accessible and all passed.

Comment 36

[Marco Zehe (:MarcoZ)]

Comment on attachment 8480396 [details] [diff] [review]
bug1031188_rev4.diff

Hi Eric,
thanks fot the updates!
A few points:
1. Please get rid of the bogus changes in nsTextEquivUtils.cpp. This file should not appear in the patch any longer since you don't touch anything after all.
2. The approach is generally good, however toolTipDescription may not be an appropriate name, since this also deals with native descriptions, but not so generic as the one you already created. I am redirecting review request to Surkov to get his final go on this.
3. Please provide a patch that includes all changes, including the test changes. It makes it easier to review the whole thing in total.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #36)

> 1. Please get rid of the bogus changes in nsTextEquivUtils.cpp. This file
> should not appear in the patch any longer since you don't touch anything
> after all.

as long as it's whitespace removal I don't really care

> 2. The approach is generally good, however toolTipDescription may not be an
> appropriate name, since this also deals with native descriptions, but not so
> generic as the one you already created. I am redirecting review request to
> Surkov to get his final go on this.

There's no need to add these two method calls

> 3. Please provide a patch that includes all changes, including the test
> changes. It makes it easier to review the whole thing in total.

agree, canceling review request until new patch then

Comment 38

[Eric Skoglund [:tephra]]

(In reply to alexander :surkov from comment #37)
> (In reply to Marco Zehe (:MarcoZ) from comment #36)
> 
> > 1. Please get rid of the bogus changes in nsTextEquivUtils.cpp. This file
> > should not appear in the patch any longer since you don't touch anything
> > after all.
> 
> as long as it's whitespace removal I don't really care
> 
Sorry still figuring out hg after only working in git. Removed the changes.

> > 2. The approach is generally good, however toolTipDescription may not be an
> > appropriate name, since this also deals with native descriptions, but not so
> > generic as the one you already created. I am redirecting review request to
> > Surkov to get his final go on this.
> 
> There's no need to add these two method calls
>
Would you care to elaborate on this? 
> > 3. Please provide a patch that includes all changes, including the test
> > changes. It makes it easier to review the whole thing in total.
> 
> agree, canceling review request until new patch then
I have now added the new patch.

Comment 39

[Eric Skoglund [:tephra]]

Attachment: revision 4

Comment 40

[alexander :surkov (:asurkov)]

Comment on attachment 8486364 [details] [diff] [review]
revision 4

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

::: accessible/generic/Accessible.cpp
@@ +283,5 @@
>    
> +  if (aDescription.IsEmpty()) {
> +    NativeDescription(aDescription);
> +    if (aDescription.IsEmpty()) {
> +      TooltipDescription(aDescription);

as I said before I don't see benefits of moving the code into separate functions

@@ -284,2 @@
>    if (!aDescription.IsEmpty()) {
> -    if (aDescription.Equals(name))

it's not against trunk still

Comment 41

[Eric Skoglund [:tephra]]

Attachment: Rev 5

Without breaking it out into different methods and still ensuring that description won't duplicate name.

Comment 42

[alexander :surkov (:asurkov)]

Comment on attachment 8489278 [details] [diff] [review]
Rev 5

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

::: accessible/generic/Accessible.cpp
@@ +327,5 @@
> +    if (nameFlag == eNameFromTooltip || aDescription.Equals(name))
> +      aDescription.Truncate();
> +
> +    return;
> +  }

why don't you call Name() in the end of the method and compare it to calculated description?

Comment 43

[Eric Skoglund [:tephra]]

(In reply to alexander :surkov from comment #42)
> Comment on attachment 8489278 [details] [diff] [review]
> Rev 5
> 
> Review of attachment 8489278 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/Accessible.cpp
> @@ +327,5 @@
> > +    if (nameFlag == eNameFromTooltip || aDescription.Equals(name))
> > +      aDescription.Truncate();
> > +
> > +    return;
> > +  }
> 
> why don't you call Name() in the end of the method and compare it to
> calculated description?

As per previous reviews I was asked to make the method return as soon as a valid description was found.

Comment 44

[alexander :surkov (:asurkov)]

(In reply to eric from comment #43)

> > why don't you call Name() in the end of the method and compare it to
> > calculated description?
> 
> As per previous reviews I was asked to make the method return as soon as a
> valid description was found.

basically that's what current code does excepting one aDescription.IsEmpty() call.

Marco, what do you think on approach should be taken.

Comment 45

[Marco Zehe (:MarcoZ)]

Comment on attachment 8489278 [details] [diff] [review]
Rev 5

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

I think I found a logic error in the patch.^

::: accessible/generic/Accessible.cpp
@@ +297,5 @@
>      }
> +  }
> +
> +  if (!aDescription.IsEmpty()) {
> +    aDescription.Truncate();

Hm, should this not be
 aDescription.CompressWhiteSpace();
instead? Otherwise the below check wouldn't make sense.

Comment 46

[Eric Skoglund [:tephra]]

(In reply to Marco Zehe (:MarcoZ) from comment #45)
> Comment on attachment 8489278 [details] [diff] [review]
> Rev 5
> 
> Review of attachment 8489278 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I found a logic error in the patch.^
> 
> ::: accessible/generic/Accessible.cpp
> @@ +297,5 @@
> >      }
> > +  }
> > +
> > +  if (!aDescription.IsEmpty()) {
> > +    aDescription.Truncate();
> 
> Hm, should this not be
>  aDescription.CompressWhiteSpace();
> instead? Otherwise the below check wouldn't make sense.

Ah yes certainly, sorry. Re-uploaded.

Comment 47

[Eric Skoglund [:tephra]]

Attachment: Rev 6

Comment 48

[Marco Zehe (:MarcoZ)]

Comment on attachment 8489389 [details] [diff] [review]
Rev 6

I'm OK with it this way, but Surkov should take another look to make sure his question is either addressed or still needs answering.

Comment 49

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #48)
> Comment on attachment 8489389 [details] [diff] [review]
> Rev 6
> 
> I'm OK with it this way, but Surkov should take another look to make sure
> his question is either addressed or still needs answering.

Marco, I think I would prefer to have two lines patch, just compare calculated description and name in the end: less changes, less code.

Comment 50

[Eric Skoglund [:tephra]]

Attachment: Rev 7

Checks if we have gotten a description and truncates it if it's the same as the name at the end of the method.

Comment 51

[alexander :surkov (:asurkov)]

Comment on attachment 8489412 [details] [diff] [review]
Rev 7

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

it is not complete solution as long as Description() is overriden by subclasses but it covers most of cases.

please upload new patch with nits fixed

::: accessible/generic/Accessible.cpp
@@ +310,5 @@
> +  if (!aDescription.IsEmpty()) {
> +    aDescription.CompressWhitespace();
> +    nsAutoString name;
> +    ENameValueFlag nameFlag = Name(name);
> +        

nit: remove whitespaces

::: accessible/tests/mochitest/test_descr.html
@@ +27,5 @@
>  
> +      // No description from aria-describedby since it is the same as the
> +      // @alt attribute which is used as the name
> +      testDescr("img4", "");
> +        

nit: whitespaces

@@ +44,5 @@
>  
>        // From title (summary is used as a name)
>        testDescr("table3", "title");
>  
> +      // No description from <desc> element since it is the same as the 

space in the end of line

@@ +101,5 @@
>    </table>
>  
>    <table id="table3" summary="summary" title="title">
>      <tr><td>cell</td></tr>
>    </table>

nit: pls put a new line between

@@ +105,5 @@
>    </table>
> +  <svg xmlns="http://www.w3.org/2000/svg" version="1.1"
> +    viewBox="0 0 100 100" preserveAspectRatio="xMidYMid slice"
> +       id="svg"
> +    style="width:100px; height:100px;">

wrong indentation (two spaces offset, here and below)

Comment 52

[Eric Skoglund [:tephra]]

Attachment: Nit fixes

Comment 53

[alexander :surkov (:asurkov)]

Comment on attachment 8489528 [details] [diff] [review]
Nit fixes

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

::: accessible/generic/Accessible.cpp
@@ +310,5 @@
> +  if (!aDescription.IsEmpty()) {
> +    aDescription.CompressWhitespace();
> +    nsAutoString name;
> +    ENameValueFlag nameFlag = Name(name);
> +        

not fixed

::: accessible/tests/mochitest/test_descr.html
@@ +27,5 @@
>  
> +      // No description from aria-describedby since it is the same as the
> +      // @alt attribute which is used as the name
> +      testDescr("img4", "");
> +        

not fixed

@@ +69,5 @@
>       href="https://bugzilla.mozilla.org/show_bug.cgi?id=666212"
>       title="summary attribute content mapped to accessible name in MSAA">
>      Mozilla Bug 666212
>    </a>
> +    <a target="_blank"

wrong indentation

@@ +104,5 @@
>      <tr><td>cell</td></tr>
>    </table>
> +  <svg xmlns="http://www.w3.org/2000/svg" version="1.1"
> +    viewBox="0 0 100 100" preserveAspectRatio="xMidYMid slice"
> +       id="svg"

not fixed

@@ +110,5 @@
> +      <title>SVG Image</title>
> +      <desc>SVG Image</desc>
> +      <linearGradient id="gradient">
> +          <stop class="begin" offset="0%"/>
> +          <stop class="end" offset="100%"/>

same

Comment 54

[Eric Skoglund [:tephra]]

Attachment: Nit fixes

Okay something must have gone wrong. This is the file I have, does it look good?

Comment 55

[alexander :surkov (:asurkov)]

Comment on attachment 8489548 [details] [diff] [review]
Nit fixes

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

yep, thanks

do you have access to land it?

Comment 56

[Eric Skoglund [:tephra]]

(In reply to alexander :surkov from comment #55)
> Comment on attachment 8489548 [details] [diff] [review]
> Nit fixes
> 
> Review of attachment 8489548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> yep, thanks
> 
> do you have access to land it?

No I dont.

Comment 57

[alexander :surkov (:asurkov)]

(needs try run)

Comment 58

[alexander :surkov (:asurkov)]

Thanks for the fix. If you feel like to make another fix you can check here https://bugzilla.mozilla.org/buglist.cgi?list_id=11178003&resolution=---&query_based_on=good-first-bug&status_whiteboard_type=substring&query_format=advanced&status_whiteboard=[good%20first%20bug]&component=Disability%20Access%20APIs&product=Core&known_name=good-first-bug

Comment 59

[Carsten Book [:Tomcat]]

(In reply to alexander :surkov from comment #57)
> (needs try run)

indeed, would be great if someone could provide a try run link and then request c-n again, thanks!

Comment 60

[alexander :surkov (:asurkov)]

(In reply to Carsten Book [:Tomcat] from comment #59)
> (In reply to alexander :surkov from comment #57)
> > (needs try run)
> 
> indeed, would be great if someone could provide a try run link and then
> request c-n again, thanks!

ok, I'll land it after running the try. It'd be good to have checking-needed option for those who doesn't have try server access.

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eab5b068f70d