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.

This patch implements the :checked pseudoclass for option elements and replaces _moz-option-selected with :checked where appropriate.

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.

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.

This patch has jst's comments addressed.

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...

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 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

this is checked in. I'm keeping the bug open because more patches will be forthcoming.

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

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.

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.

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.

sr=hyatt

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

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

nsbeta1+ per Nav triage team

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 :)

Implemented jkeiser's suggestion

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

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

Comment on attachment 73833 [details] [diff] [review] patch #2 for _moz-input-checked -> :checked a=brendan@mozilla.org for 1.0. /be

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.

Comment on attachment 74502 [details] [diff] [review] implement jkeiser's suggestion r=jkeiser

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

Comment on attachment 74502 [details] [diff] [review] implement jkeiser's suggestion a=dbaron for trunk checkin

This bug should be completely fixed now. I'll address the Txul regression over in bug 130778.