html listbox (select): Show dotted border around focused item

VERIFIED FIXED in mozilla1.0

Status

()

defect
P2
normal
VERIFIED FIXED
19 years ago
15 days ago

People

(Reporter: jruderman, Assigned: rods)

Tracking

({access, topembed+})

Trunk
mozilla1.0
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2])

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

19 years ago
Same as bug 64076, but for html form controls instead of prefs under Classic.  
Mozilla currently doesn't use the dotted border to indicate a focused listbox 
(the focused item within the listbox?), so it's difficult to see what you're 
doing on a page like http://bugzilla.mozilla.org/query.cgi when you're using 
the keyboard.  See comments in bug 64076 for more details.
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Future
(Assignee)

Comment 1

19 years ago
Futuring bug because of transition to XBL form controls

Comment 2

19 years ago
QA Contact Update
QA Contact: bsharma → vladimire

Comment 3

18 years ago
Added access and fcc508 keywords. Needed for legal compliance to FCC Section
508. Mozilla-based products need this to be used by the US government.
Keywords: access, fcc508
(Reporter)

Updated

18 years ago
Depends on: 57209

Comment 4

18 years ago
*** Bug 89466 has been marked as a duplicate of this bug. ***

Comment 5

18 years ago
summary of bug 89466 :
select box with SIZE > 1 does not display the dotted line when it receives focus

Comment 6

18 years ago
go to http://home.nbci.com/search/power/form/0,179,home-0,00.html?fd.srch.pwr

tab to the select box with title "Media Type". You will notice that the there is 
no indication of the control receiving focus. i.e.- There is  no dotted line. 

But if you use the arrow keys, you will be able to navigate within the control 
and make  your selection, too.

Comment 7

18 years ago
this is happening on all platforms
(Reporter)

Updated

18 years ago
OS: Windows 98 → All
Hardware: PC → All

Updated

18 years ago
Severity: normal → major

Comment 9

18 years ago
any progress with this bug?
When bug 40983 lands we're going to need this even more.  CTRL+up/down will
allow you to silently move the cursor (without changing the selection) up and
down the box.

Comment 11

18 years ago
Bugs affecting accessibility are now get blocker status. 

Rods, please retarget to a milestone other than FUTURE. Can you give an ETA?

Thanks
- Aaron
Severity: major → blocker
This will be fixed when XBL select is turned on.  XBL select is already in the
tree and just needs the switch flipped when it's fully tested.  I believe the
XUL list control supports this.  See bug 112713.

Comment 13

18 years ago
Right, but I've heard that XBL form controls are not 100% certain to make it
into all embedding targets we care about.

Comment 14

18 years ago
bryner said he is working on getting the XBL framing in for select, but again, I
cannot garuntee 100% that all embedding clients will use it. I'll try to get
some specifics on what the clients want.
(Assignee)

Comment 15

18 years ago
Extremely difficult, if not impossible, with the current implementation of the
list box.
This will have to wait for XBL form controls. Marking nsbeta1-.
Keywords: nsbeta1nsbeta1-

Comment 17

18 years ago
XBL form controls are nsbeta1+'d. also adding topembed keyword.
Keywords: nsbeta1-nsbeta1, topembed

Comment 18

17 years ago
Marking nsbeta1+, topembed+, m1.0. Rod, can the outline be painted in the 
overlay layer after the content, as was done in bug 105742?
Priority: -- → P2
Target Milestone: Future → mozilla1.0

Comment 19

17 years ago
Based on comment #15, reassigning to byrner and removing the pluses from 
nsbeta1+ and topembed+. If XBL form controls are at risk, then we need some lead 
time to fix this.
Assignee: rods → bryner
Status: ASSIGNED → NEW

Comment 20

17 years ago
That last comment should have read "based on comment #17"

Comment 21

17 years ago
There is some risk, since they didn't land in 0.9.9, and mozilla.org hasn't
agreed to enable them by default yet, but as long as we now have good QA
coverage on them, and people using them as dogfood, I think it still makes sense
to use them as planned.  Do we even have a fallback plan?  I thought the current
form controls hadn't been gettin' much lovin' lately...

Comment 22

17 years ago
nsbeta1+ per Nav triage team
Severity: blocker → normal
Keywords: nsbeta1nsbeta1+

Comment 23

17 years ago
Tested using the following:
3_22_05 netscape trunk
3_22_11 mfcembed trunk
3_22_11 mfcembed 0.9.9 branch

with:
http://bugzilla.mozilla.org/query.cgi

Behavior same with all applications tested. On the query page there is no dotted
line focus on the listboxes. For instance, on the six listboxes at the top
(status, resolution, platformn OpSys, priority, severity), the tab key takes you
through the boxes, but does not display with dotted borders. After tabbing out
you get the dotted border again with the 'email' link.

Updated

17 years ago
Keywords: topembedtopembed+

Updated

17 years ago
Blocks: 135206

Comment 24

17 years ago
Please update this bug with an [adt1] - [adt3] impact rating (or take it off the
list if it doesn't even rate adt3.)  Thanks!

Updated

17 years ago
Whiteboard: [adt2]

Updated

17 years ago
Blocks: 75785
This should be reassigned if the bug is to be fixed in the current(non-XBL) form
controls.

Comment 26

17 years ago
rods, for retriage
Assignee: bryner → rods
(Reporter)

Updated

17 years ago
Summary: html forms: indicate focused listbox using dotted border → html listbox (select): Show dotted border around focused item
(Assignee)

Comment 27

17 years ago
Posted file test case
Click on the checkbox to get focus to the right spot and then press <tab> to
move between the listboxes.
(Assignee)

Comment 28

17 years ago
Posted patch patch (obsolete) — Splinter Review
This displays a focus ring when the listbox has focus for HTML listboxes (NOT
XBL listboxes)

The idea is that when the SelectAreaFrame (which is the parent) of the options
is asked to paint then it asks the ListControlframe to paint the focus ring in
the correct spot. It need to find the first non-disabled option (ignoring opt
groups) and if nothing is selected it need to find the first frame which is the
dummy option. Also, it now track thru a static data member which listControl
frame currently has focus (this was taking from how comboboxes track it
intnerally)

With all the safety checks it should be low risk.
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Keywords: review

Comment 29

17 years ago
Comment on attachment 78384 [details] [diff] [review]
patch

sr=attinasi, assuming you tested all of the possible focus scenarios: actual
selection, no selection, no selection with all disabled options, no options,
etc.
Attachment #78384 - Flags: superreview+
(Assignee)

Comment 30

17 years ago
New patch with jkeiser's suggestions for getting the <ctrl> arrow keys working.
Attachment #78384 - Attachment is obsolete: true

Comment 31

17 years ago
Comment on attachment 78445 [details] [diff] [review]
patch with <ctrl>arrow keys work.

sr=attinasi
Attachment #78445 - Flags: superreview+
Comment on attachment 78445 [details] [diff] [review]
patch with <ctrl>arrow keys work.

>Index: html/forms/src/nsListControlFrame.cpp
>===================================================================
>@@ -521,6 +526,164 @@
> 
> }
> 
>+void nsListControlFrame::PaintFocus(nsIRenderingContext& aRC, nsFramePaintLayer aWhichLayer)
>+{
>+  if (NS_FRAME_PAINT_LAYER_FOREGROUND != aWhichLayer) return;
>+
>+  if (mFocused != this) return;
>+
>+  PRInt32 selectedIndex       = mEndSelectionIndex;
>+  PRInt32 actualSelectedIndex = mEndSelectionIndex;
>+  GetSelectedIndex(&actualSelectedIndex);
>+
>+  if (selectedIndex == kNothingSelected) {
>+    selectedIndex = actualSelectedIndex;
>+  }

selectedIndex isn't really selectedIndex, it's should be named focusedIndex;
then actualSelectedIndex could be changed to selectedIndex.

>+  nsIScrollableView * scrollableView;
>+  GetScrollableView(scrollableView);
>+  if (!scrollableView) return;
>+
>+  nsCOMPtr<nsIContent> content;
>+
>+  nsCOMPtr<nsIPresShell> presShell;
>+  mPresContext->GetShell(getter_AddRefs(presShell));
>+  if (!presShell) return;
>+
>+  nsIFrame* containerFrame;
>+  GetOptionsContainer(mPresContext, &containerFrame);
>+  if (!containerFrame) return;
>+
>+  nsIFrame * childframe = nsnull;
>+  nsresult result = NS_ERROR_FAILURE;
>+

Move the definition of content here; consider calling it focusedContent, but
not necessary.

>+  // If we have a selected index then get that child frame
>+  if (selectedIndex != kNothingSelected) {
>+    content = getter_AddRefs(GetOptionContent(selectedIndex));
>+    if (content) {
>+      // otherwise we find the content's frame and scroll to it
>+      if (content) {

Double if (content).

>+        result = presShell->GetPrimaryFrameFor(content, &childframe);
>+      }
>+    }
>+  } else {
>+    // Since there isn't a selected item we need to show a focus ring around the first
>+    // non-disabled item and skip all the option group elements (nodes)
>+    nsCOMPtr<nsIDOMNode> node;
>+
>+    nsCOMPtr<nsIDOMNSHTMLSelectElement> selectNSElement(do_QueryInterface(mContent));
>+    NS_ASSERTION(selectNSElement, "Can't be null");
>+
>+    nsCOMPtr<nsISelectElement> selectElement(do_QueryInterface(mContent));
>+    NS_ASSERTION(selectElement, "Can't be null");
>+
>+    nsCOMPtr<nsIDOMHTMLSelectElement> selectHTMLElement(do_QueryInterface(mContent));
>+    NS_ASSERTION(selectElement, "Can't be null");
>+
>+    PRUint32 length;
>+    selectHTMLElement->GetLength(&length);
>+    if (length) {
>+      // find the first non-disabled item
>+      PRBool isDisabled = PR_TRUE;
>+      for (PRInt32 i=0;i<PRInt32(length) && isDisabled;i++) {
>+        if (NS_FAILED(selectNSElement->Item(i, getter_AddRefs(node))) && !node) {
>+          break;
>+        }

I think the && was meant to be ||.

>+        if (NS_FAILED(selectElement->IsOptionDisabled(i, &isDisabled))) {
>+          break;
>+        }
>+        if (isDisabled) {
>+          node = nsnull;
>+        } else {
>+          break;
>+        }
>+      }
>+      if (!node) {
>+        return;
>+      }
>+    }
>+
>+    // if w found a node use, if not get the first child (this is for empty selects)
>+    if (node) {
>+      nsCOMPtr<nsIContent> optContent(do_QueryInterface(node));
>+      result = presShell->GetPrimaryFrameFor(optContent, &childframe);

I believe here you need to just use "content" so that it will be detected
below, in the optgroup detection bit.

Also what this means is the above code for (if (content) GetPrimaryFrameFor
...) can be moved down below, as can the GetPresContext and GetOptionsContainer
(assuming next comment is right).

>+    }
>+    if (!childframe) {
>+      result = containerFrame->FirstChild(mPresContext, nsnull, &childframe);

I suspect we should not be doing this--is there a reason?  If we couldn't find
the first non-disabled option to focus, do we really want to focus the first
childframe?  Not only that, but I don't think we're guaranteed the first
childframe is an option.  It could be an optgroup, for example.

>+    }
>+  }
>+
>+  if (NS_FAILED(result) || !childframe) return;
>+
>+  const nsIView * clippedView;
>+  scrollableView->GetClipView(&clippedView);
>+  if (!clippedView) return;
>+
>+  nscoord x;
>+  nscoord y;
>+  scrollableView->GetScrollPosition(x,y);
>+  // get the clipped rect
>+  nsRect rect;
>+  clippedView->GetBounds(rect);
>+
>+  // now move it by the offset of the scroll position
>+  rect.x = 0;
>+  rect.y = 0;
>+  rect.MoveBy(x,y);
>+
>+  // get the child rect
>+  nsRect fRect;
>+  childframe->GetRect(fRect);
>+
>+  float p2t;
>+  mPresContext->GetScaledPixelsToTwips(&p2t);
>+  nscoord onePixelInTwips = NSToCoordRound(p2t);
>+

Please move this closer to where it's used ...

>+  nsRect clipRect;
>+  containerFrame->GetRect(clipRect);
>+  clipRect.x = 0;
>+  clipRect.y = 0;
>+
>+  PRBool clipEmpty;
>+  aRC.PushState();
>+  aRC.SetClipRect(clipRect, nsClipCombine_kReplace, clipEmpty);
>+
>+  // adjust position is it is a child of a option group
>+  if (content) {
>+    nsRect optRect(0,0,0,0);
>+    nsCOMPtr<nsIContent> parentContent;
>+    content->GetParent(*getter_AddRefs(parentContent));
>+    nsCOMPtr<nsIDOMHTMLOptGroupElement> optGroup(do_QueryInterface(parentContent));
>+    if (optGroup) {
>+      nsIFrame * optFrame;
>+      result = presShell->GetPrimaryFrameFor(parentContent, &optFrame);
>+      if (NS_SUCCEEDED(result) && optFrame) {
>+        optFrame->GetRect(optRect);
>+      }
>+    }
>+    fRect.y += optRect.y;
>+  }
>+
>+  // set up back stop colors and then ask L&F service for the real colors
>+  nscolor color = selectedIndex >= 0?NS_RGB(245,219,149):NS_RGB(0,0,0);

Por favor, space out the ? and : a bit ... it's hard to pick out.

>+  nsCOMPtr<nsILookAndFeel> lookAndFeel;
>+  mPresContext->GetLookAndFeel(getter_AddRefs(lookAndFeel));
>+  if (actualSelectedIndex == kNothingSelected) {
>+    lookAndFeel->GetColor(nsILookAndFeel::eColor_WidgetSelectBackground, color);
>+  } else if (lookAndFeel) {
>+    lookAndFeel->GetColor(selectedIndex >= 0?nsILookAndFeel::eColor_WidgetSelectForeground:nsILookAndFeel::eColor_WidgetSelectBackground, color);
>+  }
>+
>+  nsRect dirty;
>+  nscolor colors[] = {color, color, color, color};
>+  PRUint8 borderStyle[] = {NS_STYLE_BORDER_STYLE_DOTTED, NS_STYLE_BORDER_STYLE_DOTTED, NS_STYLE_BORDER_STYLE_DOTTED, NS_STYLE_BORDER_STYLE_DOTTED};
>+  nsRect innerRect = fRect;
>+  innerRect.Deflate(nsSize(onePixelInTwips, onePixelInTwips));
>+  nsCSSRendering::DrawDashedSides(0, aRC, dirty, borderStyle, colors, fRect, innerRect, 0, nsnull);
>+
>+  aRC.PopState(clipEmpty);
>+
>+}
>+
> //---------------------------------------------------------
> // Frames are not refcounted, no need to AddRef
> NS_IMETHODIMP
>@@ -1823,6 +1986,18 @@
> nsListControlFrame::SetFocus(PRBool aOn, PRBool aRepaint)
> {
>   // XXX:TODO Make set focus work

This is no longer needed, I think you just fixed all our focus problems :)

Fix these and r=jkeiser
Attachment #78445 - Flags: review+
(Assignee)

Comment 33

17 years ago
Posted patch patch (obsolete) — Splinter Review
Made John's fixs up (mostly cosmetic)
Attachment #78445 - Attachment is obsolete: true
Comment on attachment 78485 [details] [diff] [review]
patch

r=jkeiser

Carrying sr=attinasi as well
Attachment #78485 - Flags: superreview+
Attachment #78485 - Flags: review+
(Assignee)

Updated

17 years ago
Keywords: adt1.0.0

Updated

17 years ago
Depends on: 136606
What testing have you done on this to ensure it doesn't not cause any
regressions in perf, stability or layout?

Removing adt1.0.0. Pls land this on the trunk, and let it bake for a couple of
days. If there are no regressions, and QA approves, pls renominate for ADT approval.
Keywords: adt1.0.0
(Assignee)

Comment 36

17 years ago
checked into trunk
Whiteboard: [adt2] → [adt2][trunk]
Comment on attachment 78485 [details] [diff] [review]
patch

a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #78485 - Flags: approval+
Whiteboard: [adt2][trunk] → [adt2][[FIXED_ON_TRUNK]]

Comment 38

17 years ago
Verifying fixed on trunk:
build 2002-04-12-03-trunk windows 98
and build 2002-04-12-03-trunk MacOSX
Whiteboard: [adt2][[FIXED_ON_TRUNK]] → [adt2][[VERIFIED_FIXED_ON_TRUNK]]

Comment 39

17 years ago
checkin for this bug has resulted in bug 137341
(Assignee)

Comment 40

17 years ago
Backed out the one lnie that was causing this to happen.
(Assignee)

Comment 41

17 years ago
fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Keywords: adt1.0.0
please verify this again with the one line that was backed out from the trunk to
fix the new regression.

Comment 43

17 years ago
terri, please verify this bug is fixed in the trunk.  The adt needs to know it's 
tested before the fix is checked into the branch. thanks!
(Assignee)

Comment 44

17 years ago
need to reopen doesn't work for mulitple selections
Status: RESOLVED → REOPENED
Keywords: adt1.0.0
Resolution: FIXED → ---
(Assignee)

Comment 45

17 years ago
Posted patch patch for multiple selects (obsolete) — Splinter Review
The previous patch didn't properly higfhlight selects with more than one item
selected.
Attachment #78485 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Whiteboard: [adt2][[VERIFIED_FIXED_ON_TRUNK]] → [adt2]
(Assignee)

Comment 46

17 years ago
Posted patch better patchSplinter Review
This is more streamline, it only gets the selected index if mEndSelectedIndex
is kNothingSelected
Attachment #79513 - Attachment is obsolete: true
Comment on attachment 79533 [details] [diff] [review]
better patch

+  nsCOMPtr<nsIDOMNSHTMLSelectElement>
selectNSElement(do_QueryInterface(mContent));
+  NS_ASSERTION(selectNSElement, "Can't be null");
+
+  nsCOMPtr<nsISelectElement> selectElement(do_QueryInterface(mContent));
+  NS_ASSERTION(selectElement, "Can't be null");
+

If you could move these into the if() it would be nice.

Veeeeery nice patch :)

r=jkeiser
Attachment #79533 - Flags: review+

Comment 48

17 years ago
Comment on attachment 79533 [details] [diff] [review]
better patch

sr=attinasi
Attachment #79533 - Flags: superreview+
(Assignee)

Comment 49

17 years ago
We have found now "hangs" with Win2k or Linux with today's build. fixed
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
(Assignee)

Updated

17 years ago
Whiteboard: [adt2] → [adt2][FIXED_ON_TRUNK]
(Assignee)

Comment 50

17 years ago
I can't get this to hang on Win2k or Linux. - fixed

Comment 51

17 years ago
When selecting multiple items by dragging you can see two bugs: when selecting
downward the first item gets the focus, not the last. And when selecting upwards
all of the items get the focus. I reported a separate bug 137588 about this
problem. Other then that the problem is fixed. 
Verifying on trunk builds 2002-04-16-03-trunk on Win2k,
 2002-04-17-07-trunk on Linux RedHat,
 and build 2002-04-17-03-trunk on OSX
Whiteboard: [adt2][FIXED_ON_TRUNK] → [adt2][VERIFIED_ON_TRUNK]

Comment 52

17 years ago
vfy
Status: RESOLVED → VERIFIED
Whiteboard: [adt2][VERIFIED_ON_TRUNK] → [adt2]
Keywords: adt1.0.0
adt1.0.0+ (on ADT's behalf) approval for checking into the 1.0 branch. Pls check
this in today, then add the fixed1.0.0 keyword.
Keywords: adt1.0.0adt1.0.0+
(Assignee)

Updated

17 years ago
Keywords: fixed1.0.0
(Assignee)

Comment 54

17 years ago
fixed on branch

Comment 55

17 years ago
verifying on branch from 07/17 on windows98 and linux RedHat
You need to log in before you can comment on or make changes to this bug.