[XBL Forms] Don't set attributes on HTML content nodes for internal state

RESOLVED FIXED in mozilla1.0

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: Brian Ryner (not reading))

Tracking

Trunk
mozilla1.0
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

53.32 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
34.01 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.68 KB, patch
Brian Ryner (not reading)
: review+
Ben Goodger (use ben at mozilla dot org for email)
: superreview+
brendan
: approval+
Details | Diff | Splinter Review
4.82 KB, patch
John Keiser (jkeiser)
: review+
brendan
: superreview+
dbaron
: approval+
Details | Diff | Splinter Review
(Assignee)

Description

16 years ago
Spun off from review comments on bug 112713.  We need to avoid setting
attributes on the HTML content nodes for internal state, such as which options
are selected or what option the user is hovering over.  This is a basic
correctness issue that needs to be addressed before we turn on XBL form controls.
(Assignee)

Comment 1

16 years ago
Created attachment 72560 [details] [diff] [review]
Patch to eliminate _moz-option-selected
(Assignee)

Comment 2

16 years ago
This patch implements the :checked pseudoclass for option elements and replaces
_moz-option-selected with :checked where appropriate.
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Keywords: nsbeta1
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 3

16 years ago
Created attachment 72758 [details] [diff] [review]
v2 of patch to eliminate _moz-option-selected in favor of :checked

This version adds an extra parameter to ContentStatesChanged so that we can
avoid updating the selection state every time you go in and out of :hover.
Attachment #72560 - Attachment is obsolete: true

Updated

16 years ago
Attachment #72758 - Flags: superreview+
Comment on attachment 72758 [details] [diff] [review]
v2 of patch to eliminate _moz-option-selected in favor of :checked

- In nsListControlFrame::IsContentSelected():

+  PRBool isSelected = PR_FALSE;

-  return (NS_CONTENT_ATTR_NOT_THERE == result ? PR_FALSE : PR_TRUE);
+  nsCOMPtr<nsIDOMHTMLOptionElement> optEl = do_QueryInterface(aContent);
+  if (optEl) {
+    PRBool isSelected;
+    optEl->GetSelected(&isSelected);
+  }
+
+  return isSelected;

Get rid of the locally defined isSelected that ends up shadowing the one that
you're returning.

- In nsOutlinerContentView::ContentStatesChanged():

+  aContent1->GetTag(*getter_AddRefs(contentTag));   
+  if (contentTag == nsHTMLAtoms::option) {
+    ...

You only want this to happen for HTML options, right? If so, make the if check
include contentTag->IsContentOfType(nsIContent::eHTML) as well as the check for
the option tag.

- In the last change in nsOutlinerContentView.cpp:

+  nsCOMPtr<nsIDOMHTMLOptionElement> optEl = do_QueryInterface(aContent);
+  PRBool isSelected;
+  optEl->GetSelected(&isSelected);

Add a null check for optEl, or is aContent guaranteed to be an HTML option
element here?

sr=jst with that looked over.
(Assignee)

Comment 5

16 years ago
Created attachment 72763 [details] [diff] [review]
v2.1 of patch to change _moz-option-selected to :checked

This patch has jst's comments addressed.
Attachment #72758 - Attachment is obsolete: true
(Assignee)

Comment 6

16 years ago
Sorry, that last patch didn't fix the shadowed isSelected variable, but I do
have it fixed in my tree.
Comment on attachment 72763 [details] [diff] [review]
v2.1 of patch to change _moz-option-selected to :checked

r=dbaron with the change you said you missed.

As you said on IRC, it would probably be nice to pass the state to
ContentStateChanged so you wouldn't have to go through so much work there to
figure out if it really changed...
Attachment #72763 - Flags: review+
Oh, you did.  So why do you still go through all that work in the outliner
ContentStateChanged?  If it changed, isn't it guaranteed to be the opposite of
what it used to be?

Comment 9

16 years ago
Comment on attachment 72763 [details] [diff] [review]
v2.1 of patch to change _moz-option-selected to :checked

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72763 - Flags: approval+
(Assignee)

Comment 10

16 years ago
this is checked in.  I'm keeping the bug open because more patches will be
forthcoming.
(Assignee)

Comment 11

16 years ago
Created attachment 73131 [details] [diff] [review]
patch to eliminate the "menuactive" attribute for menuitems

This patch replaces usage of the "menuactive" attribute on menuitems with a
:-moz-menuactive CSS pseudoclass.
(Assignee)

Comment 12

16 years ago
To build this patch, you will need to move nsIMenuParent.h from
layout/xul/base/src to layout/xul/base/public.  I didn't include that in the
patch because we should get the repository file copied.
Is there a way you can avoid dealing with frames in the selector matching code?
 Although it seems like it would work in this case, it also really seems ugly
since style resolution is a prerequsite for frame construction, and you'll
likely lead to a bunch of GetPrimaryFrameFor calls that have to grovel through a
partially constructed frame tree (with FindPrimaryFrameFor) and find nothing.
(Assignee)

Comment 14

16 years ago
Created attachment 73330 [details] [diff] [review]
v2 of patch to eliminate menuactive attribute for menu/menuitem

This patch allows access to the current/active item from the menubar or
menupopup content node, so the pseudoclass matching no longer needs the frames.
Attachment #73131 - Attachment is obsolete: true
Comment on attachment 73330 [details] [diff] [review]
v2 of patch to eliminate menuactive attribute for menu/menuitem

r=dbaron, although you should probably get someone more
knowledgable to review the accessible/src/xul and
layout/xul/base/src changes (perhaps your super-reviewer?).

This pattern in nsCSSStyleSheet.cpp (to which the accessible/src/xul/
change could be simplified):

>+      if (currentItem) {
>+        nsCOMPtr<nsIDOMElement> element = do_QueryInterface(mContent);
>+        if (currentItem == element)
>+          mIsMenuActive = PR_TRUE;
>+      }

could be written as the somewhat-unconventional but cleaner:

if (currentItem &&
    currentItem == nsCOMPtr<nsIDOMElement>(do_QueryInterface(mContent)))
  mIsMenuActive == PR_TRUE;

although you can feel free to disagree and keep it as-is, although
I'd rather see the accessible code and the CSSStyleSheet code use the
same pattern.
Attachment #73330 - Flags: review+

Comment 16

16 years ago
sr=hyatt

Comment 17

16 years ago
Comment on attachment 73330 [details] [diff] [review]
v2 of patch to eliminate menuactive attribute for menu/menuitem

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #73330 - Flags: approval+
(Assignee)

Comment 18

16 years ago
This patch was checked in.  I anticipate one or two additional patches needed
before closing this bug out.

Comment 19

16 years ago
nsbeta1+ per Nav triage team
Keywords: nsbeta1 → nsbeta1+
(Assignee)

Comment 20

16 years ago
Created attachment 73832 [details] [diff] [review]
Change _moz-input-checked attribute to :checked pseudoclass
Comment on attachment 73832 [details] [diff] [review]
Change _moz-input-checked attribute to :checked pseudoclass

r=jkeiser, especially if you get rid of that else and make :checked work :)
Attachment #73832 - Flags: review+
(Assignee)

Comment 22

16 years ago
Created attachment 73833 [details] [diff] [review]
patch #2 for _moz-input-checked  ->  :checked

Implemented jkeiser's suggestion
Attachment #73832 - Attachment is obsolete: true
(Assignee)

Comment 23

16 years ago
Comment on attachment 73833 [details] [diff] [review]
patch #2 for _moz-input-checked  ->  :checked

carrying over r=jkeiser
Attachment #73833 - Flags: review+
Comment on attachment 73833 [details] [diff] [review]
patch #2 for _moz-input-checked  ->  :checked

sr=ben@netscape.com
Attachment #73833 - Flags: superreview+
Comment on attachment 73833 [details] [diff] [review]
patch #2 for _moz-input-checked  ->  :checked

a=brendan@mozilla.org for 1.0.

/be
Attachment #73833 - Flags: approval+
(Assignee)

Comment 26

16 years ago
Created attachment 73994 [details] [diff] [review]
Final patch: hardcode sizetocontent behavior for selects

This patch hardcodes sizetocontent=always for any menuframe that is constructed
for a <select> element.  This is needed to avoid setting that attribute on the
HTML element.
Comment on attachment 73994 [details] [diff] [review]
Final patch: hardcode sizetocontent behavior for selects

I think this could benefit from some centralization: a method like
IsSizedToPopup(nsIContent* aContent, PRBool aRequireAlways) would make reading
the code more enjoyable.
(Assignee)

Comment 28

16 years ago
Created attachment 74502 [details] [diff] [review]
implement jkeiser's suggestion
Attachment #73994 - Attachment is obsolete: true
Comment on attachment 74502 [details] [diff] [review]
implement jkeiser's suggestion

r=jkeiser
Attachment #74502 - Flags: review+
(Assignee)

Comment 30

16 years ago
brendan, can I get sr= on this last patch? (attachment 74502 [details] [diff] [review])
Comment on attachment 74502 [details] [diff] [review]
implement jkeiser's suggestion

Nit: eliminate the sizeToPopup local in IsSizedToPopup, using early return
PR_TRUE for the select special case. Your call, sr=brendan@mozilla.org.

/be
Attachment #74502 - Flags: superreview+
Comment on attachment 74502 [details] [diff] [review]
implement jkeiser's suggestion

a=dbaron for trunk checkin
Attachment #74502 - Flags: approval+
(Assignee)

Comment 33

16 years ago
This bug should be completely fixed now.  I'll address the Txul regression over
in bug 130778.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
QA Contact: madhur → tpreston
You need to log in before you can comment on or make changes to this bug.