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

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Brian Ryner (not reading)]

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.

Comment 1

[Brian Ryner (not reading)]

Attachment: Patch to eliminate _moz-option-selected

Comment 2

[Brian Ryner (not reading)]

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

Comment 3

[Brian Ryner (not reading)]

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

Comment 4

[Johnny Stenback (:jst)]

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.

Comment 5

[Brian Ryner (not reading)]

Attachment: v2.1 of patch to change _moz-option-selected to :checked

This patch has jst's comments addressed.

Comment 6

[Brian Ryner (not reading)]

Sorry, that last patch didn't fix the shadowed isSelected variable, but I do
have it fixed in my tree.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Asa Dotzler [:asa]]

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

Comment 10

[Brian Ryner (not reading)]

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

Comment 11

[Brian Ryner (not reading)]

Attachment: patch to eliminate the "menuactive" attribute for menuitems

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

Comment 12

[Brian Ryner (not reading)]

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.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 14

[Brian Ryner (not reading)]

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

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 16

[David Hyatt]

sr=hyatt

Comment 17

[Asa Dotzler [:asa]]

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

Comment 18

[Brian Ryner (not reading)]

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

Comment 19

[Peter Trudelle]

nsbeta1+ per Nav triage team

Comment 20

[Brian Ryner (not reading)]

Attachment: Change _moz-input-checked attribute to :checked pseudoclass

Comment 21

[John Keiser (jkeiser)]

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

Comment 22

[Brian Ryner (not reading)]

Attachment: patch #2 for _moz-input-checked -> :checked

Implemented jkeiser's suggestion

Comment 23

[Brian Ryner (not reading)]

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

carrying over r=jkeiser

Comment 24

[Ben Goodger (use ben at mozilla dot org for email)]

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

sr=[e-mail]

Comment 25

[Brendan Eich [:brendan]]

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

a=[e-mail] for 1.0.

/be

Comment 26

[Brian Ryner (not reading)]

Attachment: 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 27

[John Keiser (jkeiser)]

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 28

[Brian Ryner (not reading)]

Attachment: implement jkeiser's suggestion

Comment 29

[John Keiser (jkeiser)]

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

r=jkeiser

Comment 30

[Brian Ryner (not reading)]

brendan, can I get sr= on this last patch? (attachment 74502 [details] [diff] [review])

Comment 31

[Brendan Eich [:brendan]]

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=[e-mail].

/be

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

a=dbaron for trunk checkin

Comment 33

[Brian Ryner (not reading)]

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