366340 - List items should not have SHOWING state when they are scrolled off

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Willie Walker]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a2pre) Gecko/20070107 Minefield/3.0a2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a2pre) Gecko/20070107 Minefield/3.0a2pre

List items that are not SHOWING have the SHOWING state set.  They should not have their SHOWING state set if they are not painted on the screen.  Thanks!

Reproducible: Always

Steps to Reproduce:
1. Open the attached HTML content in Firefox
2. Examine the hierarchy using at-poke

Actual Results:  
You'll see that list items and items in combo boxes have their SHOWING state set even if they are not painted on the screen.

Expected Results:  
Only the items that are intended to be painted on the screen (i.e., the ones that are visually scrolled to in the list) should have their SHOWING state set.

Comment 1

[Willie Walker]

Attachment: HTML content with lists and combobox in it

Test case.

Comment 2

[Aaron Leventhal]

SHOWING should only be set if something isn't scrolled offscreen, correct?

This is different from VISIBLE which just means that it's programatically visible, and whether or not it's currently scrolled on screen or not is not at issue for VISIBLE.

Comment 3

[Willie Walker]

(In reply to comment #2)
> SHOWING should only be set if something isn't scrolled offscreen, correct?

I believe so.  Thanks!

Comment 4

[Aaron Leventhal]

We don't use the SHOWING state correctly anywhere then -- the problem is not limited to lists.

I have implemented SHOWING in bug 367412, and that will hopefully fix this as well.

Comment 5

[Aaron Leventhal]

This is still a bug even after bug 367412. For some reason our SHOWING algorithm in nsAccessible::IsVisible() isn't working inside a listbox.

Comment 6

[Aaron Leventhal]

Is there an obvious reason this is happening?

Comment 7

[alexander :surkov (:asurkov)]

I wonder did bug 370455 fix this?

Comment 8

[Nian Liu(n/a in a long time)]

several points
1. 370455 doesn't fix it.
2. this bug is due to box model is created for all options. therefore options rectVisibility is considered to be visible. i talked with roc on irc and roc think it seems to be a bug
3. do we really need to set the SHOWING attribute rightly here for options? i don't think it gives too much help for users.

Comment 9

[Aaron Leventhal]

Can you explain #2 better? I'd be surprised if we can't tell when something is scrolled off.

Comment 10

[Aaron Leventhal]

Do we need to override this state in nsHTMLOptionAccessible::GetState()?

You could check the bounds against the parent bounds.

Comment 11

[Nian Liu(n/a in a long time)]

(In reply to comment #9)
> Can you explain #2 better? I'd be surprised if we can't tell when something is
> scrolled off.
> 

go to the test case page, use domi to see option's box model. both (x,y) and (width, height) is created. 

Comment 12

[Nian Liu(n/a in a long time)]

(In reply to comment #10)
> Do we need to override this state in nsHTMLOptionAccessible::GetState()?
> 
> You could check the bounds against the parent bounds.
> 

good idea!

still the question, does that provide more meaningful info for users?

Comment 13

[Aaron Leventhal]

Apparently it has some impact on Orca. Ask Will Walker. This is listed as "USABILITY PROBLEM - MUST BE FIXED -- Orca reads too much (i.e., items that are not showing)" at http://live.gnome.org/Orca/MozillaBugs

In addition, I expect that it affects screen magnifiers, because they may want to highlight words as they read through the list of options.

Comment 14

[Nian Liu(n/a in a long time)]

Attachment: patch

if select is collapsed, remove SHOWING state

Comment 15

[Aaron Leventhal]

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

I'm on vacation, so get reviews from surkov. Also, make sure that when the list is not collapsed, that scrolled-off items are not marked SHOWING.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> (From update of attachment 259986 [details] [diff] [review])
> Also, make sure that when the list
> is not collapsed, that scrolled-off items are not marked SHOWING.
> 

Nian, do you like to fix it in this patch?

Comment 17

[alexander :surkov (:asurkov)]

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

>+  /**
>+   * Get Select element's accessible state
>+   */ 
>+  nsIContent* GetSelectState(PRUint32* aState);

nit: document better, your method has two output arguments.

> /**
>   * As a nsHTMLSelectOptionAccessible we can have the following states:
>   *     STATE_SELECTABLE
>   *     STATE_SELECTED
>@@ -585,18 +571,25 @@ NS_IMETHODIMP nsHTMLSelectOptionAccessib
>   if ( option ) {
>     PRBool isSelected = PR_FALSE;
>     option->GetSelected(&isSelected);
>     if ( isSelected ) 
>       *_retval |= STATE_SELECTED;

Should be nsIAccessibleStates::STATE_SELECTED.

>+  nsCOMPtr<nsIDOMNode> selectNode(do_QueryInterface(content));
>+  if (selectNode) {
>+    nsCOMPtr<nsIAccessibilityService> accService(do_GetService("@mozilla.org/accessibilityService;1"));

Use GetAccService() instead.

>+    nsCOMPtr<nsIAccessible> selAcc;
>+    accService->GetAccessibleFor(selectNode, getter_AddRefs(selAcc));
>+    if (selAcc) {
>+      PRUint32 state;

unused variable


with those fixed r=me

Comment 18

[Nian Liu(n/a in a long time)]

Attachment: patch to be checked in

Comment 19

[Ginn Chen]

Attachment: patch to commit

Please update on HEAD.

Comment 20

[alexander :surkov (:asurkov)]

What's about comment #15?

Comment 21

[Nian Liu(n/a in a long time)]

it has no conflict with non-collapsed list.

or does it should be fixed in this patch if there is such a bug?

Comment 22

[Aaron Leventhal]

Look at the "Which sports do you like?" listbox in the testcase.

For me, the last item "Baseball" is scrolled off. That should not be marked SHOWING.

If it is still marked SHOWING, then this bug should be reopened and that should be fixed here. It's part of the original bug summary.

If it works already, then great!

Comment 23

[Nian Liu(n/a in a long time)]

(In reply to comment #22)
> Look at the "Which sports do you like?" listbox in the testcase.
> 
> For me, the last item "Baseball" is scrolled off. That should not be marked
> SHOWING.
> 
> If it is still marked SHOWING, then this bug should be reopened and that should
> be fixed here. It's part of the original bug summary.
> 
> If it works already, then great!
> 
it's too long for me away from the bug before i'm back to it.
i'll fix the issue with another patch

Comment 24

[Nian Liu(n/a in a long time)]

although the view is visible, the frame owns the view may not visible. looking at the test case with domi box model, we can see
list (10, 106) to (98, 160), "base ball" (10, 155) to (79, 181). note: only 160-155 = 5 is visible comparing whole height is 181-155 = 16

looking at nsAccessible::IsVisible, we can see containing view is same for all list items.

questions here is that under such scenerio, what's the rule to decide whether the accessible object is visible or not?

Comment 25

[Aaron Leventhal]

>  only 160-155 = 5 is visible comparing whole height is 181-155 = 16

Do you mean 5 pixels of the top of "Baseball" should be visible? Something must be wrong then, because I would expect at least a little bit of the tops of some of the letters. Are both sounds of coordinates relative to the same thing?

You might want to change the testcase to add a few more items to the list or descrease the height of the listbox, to make it a more useful testcase. 

Compare the bounds of the listitem with the listbox. You should require a minimal number of pixels to be visible so that it's not just the tops of letters visible. See how we do it in nsAccessible::IsVisible() using kMinPixels. Why is nsAccessible::IsVisible() not working. Is the listbox not its own view?

Comment 26

[Nian Liu(n/a in a long time)]

(In reply to comment #25)
> >  only 160-155 = 5 is visible comparing whole height is 181-155 = 16
> 
> Do you mean 5 pixels of the top of "Baseball" should be visible? 
yep
> be wrong then, because I would expect at least a little bit of the tops of some
> of the letters. Are both sounds of coordinates relative to the same thing?
> 
yep, that what domi describes
> You might want to change the testcase to add a few more items to the list or
> descrease the height of the listbox, to make it a more useful testcase. 
> 
> Compare the bounds of the listitem with the listbox. You should require a
> minimal number of pixels to be visible so that it's not just the tops of
> letters visible. See how we do it in nsAccessible::IsVisible() using
> kMinPixels. Why is nsAccessible::IsVisible() not working. Is the listbox not
> its own view?
listbox is, listbox item uses listbox's view, that's why it's always visible

Comment 27

[Aaron Leventhal]

I have posted a fix to bug 379678.

Comment 28

[Aaron Leventhal]

Fix checked in with bug 379678.