1319273 - a11y: getCharacterExtents returns 0,0 coordinates when text is empty

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Samuel Thibault]

Attachment: buggy_caret_tracking_demo_script.py

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20161115214506

Steps to reproduce:

I have attached a small python script written by Ksamak that mimics a very basic screen magnifier tracking focus location.

While running it, run firefox, press control-L to access the URL widget, empty it with backspace


Actual results:

The python script reports

position: 0,0 size 0:0


Expected results:

It should have reported at least the coordinates of the widget, just like it does when it is not empty.

I'll attach a proposed patch

Comment 1

[Samuel Thibault]

Attachment: proposed fix

Comment 2

[Vlad Bacia-Mociran [:VladB]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0

Hi Samuel. I have run the script in the terminal  after that I've started Firefox latest release v50.0 (tried also with latest Nightly v53.0a1) but there were no entries when I've press control-L to access the URL widget and emptied it with backspace. 

Can you please retest this using latest FF release and latest Nightly build (https://nightly.mozilla.org/) and report back the results ? When doing this, please use a new clean Firefox profile, maybe even safe mode, to eliminate custom settings as a possible cause (https://goo.gl/PNe90E).

Comment 3

[Samuel Thibault]

Hello,

I have just tested the nightly snapshot, the issue is still there.

By "no entries", do you mean that the test program does not emit line of output?  Do you have the at-spi bus properly running?  Does it not emit anything when going into text entries of other applications such as gedit and add/remove text in it? If not, then please recheck your accessibility configuration.

Comment 4

[Vlad Bacia-Mociran [:VladB]]

(In reply to Samuel Thibault from comment #3)
> Hello,
> 
> By "no entries", do you mean that the test program does not emit line of
> output?  Do you have the at-spi bus properly running?  Does it not emit
> anything when going into text entries of other applications such as gedit
> and add/remove text in it? If not, then please recheck your accessibility
> configuration.

Yes, the test program does not emit lines of output in the terminal. I have used this tutorial https://gnomeshell.wordpress.com/2011/08/28/manage-the-startup-applications/ to set AT SPI but still nothing for the steps in comment 0.

If I open a text file with gedit and add/delete text, the output of the program is: `position: -1,-1 size -1:-1` .

Can you provide more detailed steps on how to set the environment in order to work with Firefox/Nightly? What command do you use to run the script?

Comment 5

[Samuel Thibault]

Hello,

One thing I have in my environment that you might not be having with following that tutorial is:

export GTK_MODULES=gail:atk-bridge

to make sure that the accessibility bridges are running. But perhaps you are using gtk3?

I have to say I'm surprised: isn't there somebody at mozilla who works on accessibility and thus has the a11y environment all set up?

Samuel

Comment 6

[Vlad Bacia-Mociran [:VladB]]

Due to the fact that I cannot reproduce the issue and that the reporter can, I will assign a component to this issue. Perhaps there's someone with extensive knowledge on this area that might be able to help here.

Comment 7

[Marco Zehe (:MarcoZ)]

Joanie, Trevor, any input here?

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to Marco Zehe (:MarcoZ) from comment #7)
> Joanie, Trevor, any input here?

I'd tend to take samuel's word for it the bug exists, and looking at the code quickly tends to support that.  The substantive part of the patch seems more or less reasonable, but I'm not sure I have the time to write a test for it and get it to a state it can be committed.  Hopefully someone else can deal with that.

Comment 9

[Marco Zehe (:MarcoZ)]

Alex, can you take this and flag me for review?

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) (Returns Jan 9, 2017) from comment #9)
> Alex, can you take this and flag me for review?

It'd be nice to get thumb up on the approach from jamie or joanie. I'm good to return x, y, height of the frame, but we probably would need to make sure that width doesn't precede 0.

Otherwise, the patch looks good with minor fixes. Does Samuel has cycles to finish it?

Comment 11

[Samuel Thibault]

Attachment: patch-firefox

Comment 12

[Samuel Thibault]

Attachment: proposed fix

Here is a cleaned up patch

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 8824791 [details] [diff] [review]
proposed fix

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

thanks, Samuel for polishing this out. Would you kindly create a automated test for the changes as well? It can be added into bounds/test_zoom_text.html file.

::: firefox-esr-45.6.0esr.orig/accessible/generic/HyperTextAccessible.cpp
@@ +1252,4 @@
>      return nsIntRect();
>    }
>  
> +  if (startOffset == endOffset && startOffset == CharacterCount()) {

do I understand right that the method works when startOffset == endOffset && startOffset < CharacterCount()?

@@ +1255,5 @@
> +  if (startOffset == endOffset && startOffset == CharacterCount()) {
> +    // Empty content, use our own bound to at least get x,y coordinates
> +    bounds = GetFrame()->GetScreenRect();
> +    if (bounds.width < 0)
> +      bounds.width = 0;

is this a case?

Comment 14

[Samuel Thibault]

(In reply to alexander :surkov from comment #13)
> thanks, Samuel for polishing this out. Would you kindly create a automated
> test for the changes as well? It can be added into
> bounds/test_zoom_text.html file.

Well, I have to say I don't even know how to run this test. I'm usually just building the debian package, and that doesn't seem to be running that test (at least that I can find the log), and just opening the html file inside firefox brings "TypeError: nsIAccessibleStates is undefined", "ReferenceError: SimpleTest is not defined" etc.

> ::: firefox-esr-45.6.0esr.orig/accessible/generic/HyperTextAccessible.cpp
> @@ +1252,4 @@
> >      return nsIntRect();
> >    }
> >  
> > +  if (startOffset == endOffset && startOffset == CharacterCount()) {
> 
> do I understand right that the method works when startOffset == endOffset &&
> startOffset < CharacterCount()?

Yes. 

> @@ +1255,5 @@
> > +  if (startOffset == endOffset && startOffset == CharacterCount()) {
> > +    // Empty content, use our own bound to at least get x,y coordinates
> > +    bounds = GetFrame()->GetScreenRect();
> > +    if (bounds.width < 0)
> > +      bounds.width = 0;
> 
> is this a case?

Well, I don't know, that was just meant to answer "we probably would need to make sure that width doesn't precede 0."

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Samuel Thibault from comment #14)

> > @@ +1255,5 @@
> > > +  if (startOffset == endOffset && startOffset == CharacterCount()) {
> > > +    // Empty content, use our own bound to at least get x,y coordinates
> > > +    bounds = GetFrame()->GetScreenRect();
> > > +    if (bounds.width < 0)
> > > +      bounds.width = 0;
> > 
> > is this a case?
> 
> Well, I don't know, that was just meant to answer "we probably would need to
> make sure that width doesn't precede 0."

I meant that a frame with no text may have non zero width, for example, <p style="width: 100em; height: 1em;"></p>. In this case we may return non 0 width for (0, 0) range. Sounds about right?

Comment 16

[Samuel Thibault]

(In reply to alexander :surkov from comment #15)
> (In reply to Samuel Thibault from comment #14)
> 
> > > @@ +1255,5 @@
> > > > +  if (startOffset == endOffset && startOffset == CharacterCount()) {
> > > > +    // Empty content, use our own bound to at least get x,y coordinates
> > > > +    bounds = GetFrame()->GetScreenRect();
> > > > +    if (bounds.width < 0)
> > > > +      bounds.width = 0;
> > > 
> > > is this a case?
> > 
> > Well, I don't know, that was just meant to answer "we probably would need to
> > make sure that width doesn't precede 0."
> 
> I meant that a frame with no text may have non zero width, for example, <p
> style="width: 100em; height: 1em;"></p>. In this case we may return non 0
> width for (0, 0) range. Sounds about right?

Ah, OK. Well, yes, it makes sense.

Comment 17

[Samuel Thibault]

Attachment: proposed fix

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Samuel Thibault from comment #17)
> Created attachment 8825500 [details] [diff] [review]
> proposed fix

Samuel, do you have time to add tests to your patch? see comment #13

Comment 19

[Samuel Thibault]

Well, I could take the time, but as I said in comment #14, I don't know how to run the test: just opening the page does not seem to be working.

Comment 20

[Samuel Thibault]

Just to be more specific: I open the test_zoom_text.html file in firefox, and in the JS console I get:

The Components object is deprecated. It will soon be removed. common.js:4:6
TypeError: nsIAccessibleStates is undefined common.js:74:7
TypeError: nsIAccessibleRole is undefined role.js:4:7
TypeError: Components.utils is undefined browser.js:104:1
ReferenceError: SimpleTest is not defined test_zoom_text.html:57:5

I don't know if I'm supposed to pass some args to the browser to get it to include some stuff needed for the tests, testing/README.txt is not really verbose in that regard :)

Comment 21

[Samuel Thibault]

Attachment: proposed fix

Ok, I managed to find my way between mosh, the different way Debian builds firefox, and missing tools, so here is a tested patch, I'll also attach the test result with and without the fix.

Comment 22

[Samuel Thibault]

Attachment: success log

Comment 23

[Samuel Thibault]

Attachment: fail log

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 8843724 [details] [diff] [review]
proposed fix

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

thank you for sorting out the test stuff, the patch looks good, however I'm curious about width issue (see below)

::: accessible/tests/mochitest/bounds/test_zoom_text.html
@@ +36,5 @@
> +      var formNode = aDoc.getElementById(aContainerID);
> +
> +      var [x, y, width, height] = getBounds(formNode);
> +      testTextBounds(formNode, 0, -1, [x, y, width, height],
> +                     COORDTYPE_SCREEN_RELATIVE);

shouldn't we return a frame's width for collapsed range, for example, 236 for (0, 0) in this example, shouldn't it be 0?

@@ +61,5 @@
>        "<meta http-equiv='Content-Type' content='text/html;charset=utf-8'>" +
>        "</meta><body>" +
>        "<p id='p1' style='font-family: monospace;'>Tilimilitryamdiya</p>" +
> +      "<p id='p2'>ل</p>"+
> +      "<form><input id='i1'>foo</input></form>"+

input doens't pick up 'foo' from inner text, so <input id='i1'> would be enough

Comment 25

[Samuel Thibault]

Bugzilla@Mozilla, on mar. 07 mars 2017 14:40:51 +0000, wrote:
> ::: accessible/tests/mochitest/bounds/test_zoom_text.html
> @@ +36,5 @@
> > +      var formNode = aDoc.getElementById(aContainerID);
> > +
> > +      var [x, y, width, height] = getBounds(formNode);
> > +      testTextBounds(formNode, 0, -1, [x, y, width, height],
> > +                     COORDTYPE_SCREEN_RELATIVE);
>
> shouldn't we return a frame's width for collapsed range, for example, 236 for
> (0, 0) in this example, shouldn't it be 0?

I'm afraid I don't understand most of this. What do you call "frame"?
What do you call "collapsed"?  What do you refer to with (0, 0)?

> @@ +61,5 @@
> >        "<meta http-equiv='Content-Type' content='text/html;charset=utf-8'>" +
> >        "</meta><body>" +
> >        "<p id='p1' style='font-family: monospace;'>Tilimilitryamdiya</p>" +
> > +      "<p id='p2'>ل</p>"+
> > +      "<form><input id='i1'>foo</input></form>"+
>
> input doens't pick up 'foo' from inner text, so <input id='i1'> would be enough

Sure, but it'd then be <input id='i1'/> to be XML-compliant, right?

Comment 26

[Samuel Thibault]

Attachment: patch

Ok, reading again, I guess I understand that you mean that when requesting start=end=0, we should also get the frame width? That's indeed what we get with the proposed patch. In the attached patch, I add that testcase too. I also dropped the spurious "foo" text.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to Samuel Thibault from comment #26)
> Created attachment 8847334 [details] [diff] [review]
> patch
> 
> Ok, reading again, I guess I understand that you mean that when requesting
> start=end=0, we should also get the frame width? That's indeed what we get
> with the proposed patch. 

Sorry for the late answer. Actually I meant the opposite. The method returns a rect for a given substring, correct? I argue that it should return 0 width for collapsed ranges. Otherwise, rect of (0, 0) is greater than rect of (0, 1) in case of <p>hello</p>, which doesn't make sense, because rect of a greater range cannot be smaller. Agree?

Comment 28

[Samuel Thibault]

Attachment: patch

Well, in the case at stake, the content is empty, so there is no such thing as (0,1) range :) And since the content is empty, there is no child to tell us where the empty range should appear anyway, and thus returning the whole frame rectangle makes sense.

Now, it indeed makes sense to fix the function for empty ranges of non-empty content too, indeed. In the revised attached patch, I separated the case of empty content, the case of empty range at the beginning of the text (and thus we want a zero-width rectangle on the left of the first character), and the case of empty range not at the beginning of the text (and thus we want a zero-width rectangle on the right of the previous character).

I however couldn't find out how to properly test it: how to reach the coordinates of the first character of the context of the input? I have left the idea of the test, just commented out its invocation

Comment 29

[ksamak]

No new insight on this bug?
It would add better stability to compiz/ezoom screen magnifier, and potentially support for more widgets and items.
thanks,

Comment 30

[alexander :surkov (:asurkov)]

(In reply to Samuel Thibault from comment #28)
> Created attachment 8849759 [details] [diff] [review]
> patch
> 
> Well, in the case at stake, the content is empty, so there is no such thing
> as (0,1) range :) And since the content is empty, there is no child to tell
> us where the empty range should appear anyway, and thus returning the whole
> frame rectangle makes sense.

I don't recall which patch I gave (0, 1) example for, it's not applied to the latest one for sure. Anyway is there a practical value of returning whole element's width for no text elements? It feels a contradiction that empty range of empty element is a rect of element's width and height, however empty range of non empty element is a rect of 0 width and element's height.

> Now, it indeed makes sense to fix the function for empty ranges of non-empty
> content too, indeed. In the revised attached patch, I separated the case of
> empty content, the case of empty range at the beginning of the text (and
> thus we want a zero-width rectangle on the left of the first character), and
> the case of empty range not at the beginning of the text (and thus we want a
> zero-width rectangle on the right of the previous character).

Is the existing code fails to compute height properly in case of collapsed ranges?

> I however couldn't find out how to properly test it: how to reach the
> coordinates of the first character of the context of the input? I have left
> the idea of the test, just commented out its invocation

you could try to have a one character/no style container and use getBoundingClientRect or getClientRectsAndTexts method, which is chrome only, and may require chrome tests, see https://dxr.mozilla.org/mozilla-central/source/dom/base/test/chrome/test_range_getClientRectsAndTexts.html?q=test_range_getClientRectsAndTexts.html&redirect_type=direct

Comment 31

[Samuel Thibault]

The existing code indeed can't compute the height property, because since the text is empty, there are no children to get the height of.

Comment 32

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 33

[Samuel Thibault]

Attachment: 0-0-text-empty

Getting back to this issue.

As mentioned before, I don't know how javascript enough to properly test the case when the requested selection is empty for a non-empty entry. I'm afraid I don't have time to spend on diving in javascript etc.

I'm thus here re-submitting the patch that actually fixes the only issue at stake: when the entry is completely empty we don't have any bbox more precise than the whole entry anyway, which is however completely fine for screen magnification purposes.

The corner case that the previous proposed fix was handling (empty selection for a non-empty entry) does not happen in practice: atk errors out before reaching this, for instance.

Comment 34

[Samuel Thibault]

Uh? I asked for reopening the bug, and it got marked as "verified"!?
(and now I can't reopen it...)

Comment 35

[Samuel Thibault]

I have also posted the patch to the try server, enabling just mochitests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a5172de3163908781d0355b5c6d5ae743ccbf25&selectedJob=179839120

it is appearing as successful in the M-e10s bc4 part

Comment 36

[alexander :surkov (:asurkov)]

Comment on attachment 8980247 [details] [diff] [review]
0-0-text-empty

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

::: accessible/generic/HyperTextAccessible.cpp
@@ +1245,5 @@
> +  if (CharacterCount() == 0) {
> +    nsPresContext* presContext = mDoc->PresContext();
> +    // Empty content, use our own bound to at least get x,y coordinates
> +    return GetFrame()->GetScreenRectInAppUnits().
> +      ToNearestPixels(presContext->AppUnitsPerDevPixel());

I have dejavu on this code :) but I don't see anything in the code. We might have some open bugs on this.

Anyway why do we return a frame width? I think it's possible to get a width of CharCount() == 0 bigger than width of CharCount() > 0, which look confusing.

@@ +1249,5 @@
> +      ToNearestPixels(presContext->AppUnitsPerDevPixel());
> +  }
> +
> +  if (startOffset == endOffset) {
> +    NS_ERROR("Wrong in offset");

why is it considered wrong? the caller could use it to detect height of text

Comment 37

[Samuel Thibault]

(In reply to alexander :surkov from comment #36)
> why do we return a frame width? I think it's possible to get a width
> of CharCount() == 0 bigger than width of CharCount() > 0, which look
> confusing.

Well, yes that will happen. Ideally we would return a zero-width bbox having the height of the first character that would be typed, but since with CharCount() == 0 there is no child character, there is nothing to get the height of.

Also, it can be argued that it sometimes happen that with CharCount() == 1 ("m") you have a bigger frame than with CharCount() == 2 ("ll") :)

> @@ +1249,5 @@
> > +      ToNearestPixels(presContext->AppUnitsPerDevPixel());
> > +  }
> > +
> > +  if (startOffset == endOffset) {
> > +    NS_ERROR("Wrong in offset");
> 
> why is it considered wrong? the caller could use it to detect height of text

Yes, but that is not implemented yet, it currently just returns (0,0,0,0), so better warn about it instead of just returning an odd bbox. It happens that in atk itself that case is even rejected.

Comment 38

[Cosmin Sabou [:CosminS]]

This patch failed to apply. Please take a look. 

applying 0-0-text-empty
patching file accessible/tests/browser/bounds/browser_test_zoom_text.js
Hunk #1 FAILED at 15
1 out of 2 hunks FAILED -- saving rejects to file accessible/tests/browser/bounds/browser_test_zoom_text.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0-0-text-empty

Comment 39

[Samuel Thibault]

Attachment: patch

Ah, sorry, there had been changes in between indeed, here is the fixed patch (which was the one sent to the try server)

Comment 40

[Samuel Thibault]

Comment on attachment 8980517 [details] [diff] [review]
patch

I guess for being committed this is missing a review+ from alexander

Comment 41

[alexander :surkov (:asurkov)]

Comment on attachment 8980517 [details] [diff] [review]
patch

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

r=me, but you don't have to re-request review for carryovers

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f585271d250d
Accessible: Make TextBounds return rect of whole frame if content is empty. r=surkov

Comment 43

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c58416b4af6
Backed out changeset f585271d250d for eslint failures on /gecko/accessible/tests/browser/bounds/browser_test_zoom_text.js on a CLOSED TREE

Comment 44

[Raul Gurzau (:RaulG)]

Backed out changeset f585271d250d (bug 1319273) for eslint failures on /gecko/accessible/tests/browser/bounds/browser_test_zoom_text.js on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/9c58416b4af610a00a9fb7b1344659afeb13ba60

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f585271d250d8750240e8ab4ec8e462e56eac21e

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=181398312&repo=mozilla-inbound&lineNumber=263

Log snippet: 

[vcs 2018-06-01T19:09:05.581Z] TinderboxPrint:<a href=https://hg.mozilla.org/integration/mozilla-inbound/rev/f585271d250d8750240e8ab4ec8e462e56eac21e title='Built from mozilla-inbound revision f585271d250d8750240e8ab4ec8e462e56eac21e'>f585271d250d8750240e8ab4ec8e462e56eac21e</a>
[task 2018-06-01T19:09:05.581Z] executing ['bash', '-cx', 'cd /builds/worker/checkouts/gecko/ && cp -r /build/node_modules_eslint node_modules && ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules && ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules && ./mach lint -l eslint -f treeherder --quiet\n']
[task 2018-06-01T19:09:05.584Z] + cd /builds/worker/checkouts/gecko/
[task 2018-06-01T19:09:05.584Z] + cp -r /build/node_modules_eslint node_modules
[task 2018-06-01T19:09:05.705Z] + ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules
[task 2018-06-01T19:09:05.706Z] + ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules
[task 2018-06-01T19:09:05.708Z] + ./mach lint -l eslint -f treeherder --quiet
[task 2018-06-01T19:09:06.326Z] New python executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python2.7
[task 2018-06-01T19:09:06.326Z] Also creating executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python
[task 2018-06-01T19:09:07.965Z] Installing setuptools, pip, wheel...done.
[task 2018-06-01T19:09:09.005Z] running build_ext
[task 2018-06-01T19:09:09.005Z] building 'psutil._psutil_linux' extension
[task 2018-06-01T19:09:09.005Z] creating build
[task 2018-06-01T19:09:09.005Z] creating build/temp.linux-x86_64-2.7
[task 2018-06-01T19:09:09.005Z] creating build/temp.linux-x86_64-2.7/psutil
[task 2018-06-01T19:09:09.005Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-06-01T19:09:09.005Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-06-01T19:09:09.005Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o
[task 2018-06-01T19:09:09.005Z] creating build/lib.linux-x86_64-2.7
[task 2018-06-01T19:09:09.005Z] creating build/lib.linux-x86_64-2.7/psutil
[task 2018-06-01T19:09:09.005Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so
[task 2018-06-01T19:09:09.005Z] building 'psutil._psutil_posix' extension
[task 2018-06-01T19:09:09.005Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-06-01T19:09:09.005Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-06-01T19:09:09.005Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2018-06-01T19:09:09.006Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-06-01T19:09:09.006Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-06-01T19:09:09.006Z] 
[task 2018-06-01T19:09:09.006Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-06-01T19:14:15.210Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/accessible/tests/browser/bounds/browser_test_zoom_text.js:23:5 | Unexpected var, use let or const instead. (mozilla/var-only-at-top-level)
[task 2018-06-01T19:14:15.211Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/accessible/tests/browser/bounds/browser_test_zoom_text.js:25:5 | Unexpected var, use let or const instead. (mozilla/var-only-at-top-level)
[taskcluster 2018-06-01 19:14:15.545Z] === Task Finished ===
[taskcluster 2018-06-01 19:14:15.545Z] Unsuccessful task run with exit code: 1 completed in 561.266 seconds

Comment 45

[alexander :surkov (:asurkov)]

Samuel, do you run the patches via try server before checkin requests? It makes sense to do. If you don't have access to the try server, then you probably should request it.

Comment 46

[Samuel Thibault]

Yes I did, as I mentioned in comment #35, this is the try attempt, which does show browser_test_zoom_text.js success e.g. in linux x64 opt's M-e10s bc4 part:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a5172de3163908781d0355b5c6d5ae743ccbf25&selectedJob=179839120

    TEST-OK | accessible/tests/browser/bounds/browser_test_zoom_text.js | took 423ms

I'll dig more later.

Comment 47

[Samuel Thibault]

Attachment: patch

Ok, it's actually a lint check, which indeed wasn't enabled in my try attempt (I enabled only the mochitest on the https://mozilla-releng.net/trychooser/ form to save try server time, I assumed it'd do everything for that part of the tests).
I have updated the patch, and launched

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dad689fac8e3de6616cdba674215ff4daf1b243

where I just enabled all tests, just not on absolutely all platforms. At least it seems it has triggered the Lint part which posed problem.

Comment 48

[Samuel Thibault]

Attachment: patch

Looking more closely at test results, it seems startOffset==endOffset does happen on windows even when the content is not empty. I don't see how that case can be useful since the code would then return (0,0,0,0), but oh well, let's just avoid bothering people, I have dropped the check from the patch, and resubmitted on

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e7e0c4c333a1d1ae63a7ecee41593e0a0403153

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea974e9f568
Accessible: Make TextBounds return rect of whole frame if content is empty. r=surkov

Comment 50

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9ea974e9f568