1341762 - add nsTextFrame::HasRenderedText method

Status: NEW

(Core :: Layout, defect)

Description

[alexander :surkov (:asurkov)]

a11y doesn't always need to have a whole computed text. It just needs to know whether there's a rendered text for a text node or not. I realize that HasRenderedText logic could be much simpler than GetRenderedText.

Any recommendations or help on implementation of the method since I'm not familiar with this code?

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

It's not clear to me what would you expect to get from this method. Could you explain in what condition should it return true?

Comment 2

[Mats Palmgren (inactive)]

I'm fairly strongly against adding new methods specifically for a11y in Layout.
IMHO, it would be better if a11y didn't use the frame tree at all and instead
got all the data it needs through DOM.

We recently added a getClientRectsAndTexts() method in bug 1314080, is there
any reason you can't use that instead?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #1)
> It's not clear to me what would you expect to get from this method. Could
> you explain in what condition should it return true?

It should return true whenever GetRenderedText(0, Max) returns non empty string, in other words, if a text frame has any rendered text, then it returns true, if a text frame doesn't render any text, then false.

(In reply to Mats Palmgren (:mats) from comment #2)
> I'm fairly strongly against adding new methods specifically for a11y in
> Layout.
> IMHO, it would be better if a11y didn't use the frame tree at all and instead
> got all the data it needs through DOM.

a11y is about semantics which makes it quite close to DOM for sure, but same time a11y is closely tight to what the user sees. For example, if something is not rendered, then it's not exposed via a11y. Same is applicable to the text. If you do
<span text-transform: uppercase;>hello</span>
then a11y is expected to expose 'HELLO'.
If a text is not rendered, for example, whitespaces at certain circumstances, then a11y says there's no text.

> We recently added a getClientRectsAndTexts() method in bug 1314080, is there
> any reason you can't use that instead?

as far as I can see, it returns a text from content, not rendered one, which makes it unsuitable for a11y

Comment 4

[alexander :surkov (:asurkov)]

The method looks definitely helpful for a11y perfromance, and it doesn't seem making the a11y-layout dependence any worse. Mats, do you still have concerns?

Comment 5

[Mats Palmgren (inactive)]

> as far as I can see, it returns a text from content, not rendered one, which makes it unsuitable for a11y

Ah, but that might just be a mistake.  Or, if it's not, we could add an option
to get the rendered text instead through the same API.
Would that work for your needs?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Mats Palmgren (:mats) from comment #5)
> > as far as I can see, it returns a text from content, not rendered one, which makes it unsuitable for a11y
> 
> Ah, but that might just be a mistake.  Or, if it's not, we could add an
> option
> to get the rendered text instead through the same API.
> Would that work for your needs?

Do you mean via DOM range API? It's probably not that helpful in existing architecture, because a11y deals with text frames/text nodes, and DOM ranges will be an extra layer to deal with.

For this specific bug I wanted a method that will say whether a text node/text frame has any rendered text or not. I definitely could do GetRenderedText(0, Max) via or ranges API, if it supported rendered text option, but I realize this probably won't be a fastest way to do this.

Comment 7

[Mats Palmgren (inactive)]

(In reply to alexander :surkov from comment #6)
> I definitely could do GetRenderedText(0, Max) via or ranges API, if it supported rendered text option...

It was just a bug - we have fixed that now in bug 1343695.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Mats Palmgren (:mats) from comment #7)
> (In reply to alexander :surkov from comment #6)
> > I definitely could do GetRenderedText(0, Max) via or ranges API, if it supported rendered text option...
> 
> It was just a bug - we have fixed that now in bug 1343695.

Ok, what about the performance part? Say, I have a text node and I need to know whether it has rendered text or not. The ranges API probably won't be the fastest way, correct?

Comment 9

[Mats Palmgren (inactive)]

I don't think setting up a temporary nsRange adds any significant overhead.
The performance should be about the same as calling nsTextFrame::GetRenderedText
directly.  For typical web pages a text node doesn't have very much text so
getting the text shouldn't be much of a performance problem unless you call
it on very hot paths for some reason.

Comment 10

[alexander :surkov (:asurkov)]

We run the check for each text node in a DOM document, when building the accessible tree, as well for all text nodes from mutating subtrees, when updating the accessible tree, which are heavy operations overall. I'm trying to get rid of all bulky checks, whenever I can.

Comment 11

[Mats Palmgren (inactive)]

Is there any reason you need to have this information up front for all nodes?
Can you do the call on demand for each node when you need it instead?

Comment 12

[alexander :surkov (:asurkov)]

No, at least not in the foreseeable future: the screen readers tend to grab the whole accessible tree at a time. Also it probably won't work with existing e10s a11y architecture, when the main process caches many stuff from content processes.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit BugBot documentation.