Closed Bug 366340 Opened 15 years ago Closed 14 years ago

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

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: wwalker, Assigned: aaronlev)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(3 files, 1 obsolete file)

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.
Keywords: access
Test case.
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.
Assignee: nobody → aaronleventhal
Blocks: keya11y
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis
Version: unspecified → Trunk
(In reply to comment #2)
> SHOWING should only be set if something isn't scrolled offscreen, correct?

I believe so.  Thanks!
Depends on: statesa11y
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
This is still a bug even after bug 367412. For some reason our SHOWING algorithm in nsAccessible::IsVisible() isn't working inside a listbox.
Blocks: htmla11y
No longer blocks: keya11y
Summary: In AT-SPI implementation, List items have SHOWING state set when they are not SHOWING → List items should not have SHOWING state when they are scrolled off
Blocks: orca
Is there an obvious reason this is happening?
Severity: normal → major
I wonder did bug 370455 fix this?
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.
Can you explain #2 better? I'd be surprised if we can't tell when something is scrolled off.
Do we need to override this state in nsHTMLOptionAccessible::GetState()?

You could check the bounds against the parent bounds.
(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. 
(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?
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.
Attached patch patchSplinter Review
if select is collapsed, remove SHOWING state
Attachment #259986 - Flags: review?(aaronleventhal)
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.
Attachment #259986 - Flags: review?(aaronleventhal) → review?(surkov.alexander)
(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 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
Attachment #259986 - Flags: review?(surkov.alexander) → review+
Assignee: aaronleventhal → nian.liu
Attached patch patch to commitSplinter Review
Please update on HEAD.
Attachment #261109 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
it has no conflict with non-collapsed list.

or does it should be fixed in this patch if there is such a bug?
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!
(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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
>  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?
(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

Assignee: nian.liu → aaronleventhal
Status: REOPENED → NEW
I have posted a fix to bug 379678.
Depends on: 379678
Status: NEW → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Fix checked in with bug 379678.
You need to log in before you can comment on or make changes to this bug.