Closed Bug 708874 Opened 13 years ago Closed 12 years ago

API for locking pseudo-class state of an element

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: harth, Assigned: harth)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 3 obsolete files)

Just in case you want to see what I had so far
10.07 KB, patch
Details | Diff | Splinter Review
API for locking element's style state v2
24.98 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
API for locking element's style state v2, updated to comments
27.16 KB, patch
Details | Diff | Splinter Review 
For bug 707740 we need a way to lock down and unlock the state of an element. DOMUtils' setContentState() has an API for this, but :hover, :active, :focus are reset when another element grabs them.

We'd want to be able to lock the state for a particular element, while still allowing the regular mouse events on the page.
Do you want to lock all state, or just a particular state bit or subset of bits?  I assume it's the latter, right?
Component: General → DOM
QA Contact: general → general
Also, do you need this for arbitrary state bits, or just hover/active/focus/focusring?
Version: unspecified → Trunk
(In reply to Boris Zbarsky (:bz) from comment #1)
> Do you want to lock all state, or just a particular state bit or subset of
> bits?  I assume it's the latter, right?

Yes, we'd just want to lock subsets.

(In reply to Boris Zbarsky (:bz) from comment #2)
> Also, do you need this for arbitrary state bits, or just
> hover/active/focus/focusring?

At the least :hover :active :focus :visited. (:link, :enabled, :disabled, :valid, :invalid, :checked would be nice, but only if they come for basically free)

We'd want to be able to unset those as well.
OK, thanks.  Let me think about how to do this.
Seems like we could do something like:
 * have a bitfield of "locked states" in the event state manager
 * bail out of SetContentState right at the top if we hit the bitfield
 * provide an API that provides:
   * lockEventState(state, element), which would:
     (1) clear the bit from the bitfield
     (2) call SetContentState(state, element)
     (3) set the bitfield
   * unlockEventState(state), which would;
     (1) clear the bit from the bitfield
     (2) call setContentState(state, null) [or maybe something smarter?]

That sort of thing would work for :hover and :active, but :focus might be harder.

Toggling :link/:visited state would be entirely separate.
Hmm.  I interpreted the last bit of comment 0 to mean that we want to lock one element into :hover while allowing other elements to go in/out of :hover as normal.  Heather, was that what you meant?  Or do you want to allow mouse event _handlers_ to fire but not :hover changes to other elements?
(In reply to Boris Zbarsky (:bz) from comment #6)
> Hmm.  I interpreted the last bit of comment 0 to mean that we want to lock
> one element into :hover while allowing other elements to go in/out of :hover
> as normal.  Heather, was that what you meant?  Or do you want to allow mouse
> event _handlers_ to fire but not :hover changes to other elements?

We want other elements to be able to :hover as usual.
To be clear, for :focus - setting it in this API should not make the element the focused element on the page, it should just apply :focus styles.

Same goes for :visited; it shouldn't mark the page visited in history.
To sum up: setting the pseudo-class state in this API should just change the styles, but should be deep enough that DOMUtils' getCSSStyleRules() contains the pseudo-class rules that have been applied by this API.
(In reply to Heather Arthur [:harth] from comment #7)
> (In reply to Boris Zbarsky (:bz) from comment #6)
> > Hmm.  I interpreted the last bit of comment 0 to mean that we want to lock
> > one element into :hover while allowing other elements to go in/out of :hover
> > as normal.  Heather, was that what you meant?  Or do you want to allow mouse
> > event _handlers_ to fire but not :hover changes to other elements?
> 
> We want other elements to be able to :hover as usual.

I'm a little skeptical of this:  might it be better to avoid letting authors debug a situation that can't actually happen?
(In reply to David Baron [:dbaron] from comment #10)
> (In reply to Heather Arthur [:harth] from comment #7)
> > We want other elements to be able to :hover as usual.
> 
> I'm a little skeptical of this:  might it be better to avoid letting authors
> debug a situation that can't actually happen?

It might be worse to change the behavior of the rest of the page so that it temporarily doesn't respond as expected to some mouse movements. And as this is just for the purpose of inspecting styles I think it's okay to change the element only on a visual level.
Right. This is kind of an artificial environment for debugging the styles of an element that might only be visible when some set of states is present on the page. Right now, it sounds like the inIDOMUtils::getContentState and setContentState don't really go far enough for what we'd like them to do.

Other tools, like Firebug are currently tweaking these states and then raising a nested event loop to prevent further interactions with the page. This seems like a very heavy hammer for this particular nail.
Btw. setContentState() doesn't seem to work for :focus, while all other states defined at https://developer.mozilla.org/en/XPCOM_Interface_Reference/inIDOMUtils#Content_state_flags work fine.
I tried that to fix http://code.google.com/p/fbug/issues/detail?id=3407 for Firebug.

Sebastian
Ah, and I forgot to mention, that we already have a Firebug issue covering the same at http://code.google.com/p/fbug/issues/detail?id=3230.

Sebastian
Assignee: nobody → fayearthur
Status: NEW → ASSIGNED
> Btw. setContentState() doesn't seem to work for :focus

Yeah, it hasn't in a while.

So my current plan is an API like this:

  void lockElementState(in nsIDOMElement element, in DOMString pseudoclass
                        [optional] in boolean lockOff);
  void unlockElementState(in nsIDOMElement element, in DOMString pseudoclass);

where you would lock an element into :hover like so:

  lockElementState(myElement, ":hover");

and lock an element into not-:hover like so:

  lockElementState(myElement, ":hover", true);

The other option is having separate functions for locking a state on and off.  Preferences?
(In reply to Boris Zbarsky (:bz) from comment #15)
> > Btw. setContentState() doesn't seem to work for :focus
> 
> Yeah, it hasn't in a while.
> 
> So my current plan is an API like this:
> 
>   void lockElementState(in nsIDOMElement element, in DOMString pseudoclass
>                         [optional] in boolean lockOff);
>   void unlockElementState(in nsIDOMElement element, in DOMString
> pseudoclass);
> 
> where you would lock an element into :hover like so:
> 
>   lockElementState(myElement, ":hover");
> 
> and lock an element into not-:hover like so:
> 
>   lockElementState(myElement, ":hover", true);
> 
> The other option is having separate functions for locking a state on and
> off.  Preferences?

fwiw, I have already implemented this, and post a patch soon if that's okay? The API I have right now (in DOMUtils) is:

lockPseudoClass(element, ":hover");
unlockPseudoClass(element, ":hover");
hasPseudoClassLock(element, ":hover");
clearPseudoClassLocks(element);

I'm actually using nsEventStates temporarily instead of ":hover" until I figure out cpp strings, but that's the plan. This API is what I think we need for the devtools UI.
> fwiw, I have already implemented this, and post a patch soon if that's okay?

Oh, great.  I'll stop working on it for now, then.  ;)  Happy to review the patch.

It might be worth it to clear a single lock at a time, in addition to having a way to clear them all.

> I'm actually using nsEventStates temporarily

Here's what I have in my tree:

static nsEventStates
GetStatesForPseudo(const nsAString& aStatePseudo)
{
  // An array of the states that are relevant for various pseudoclasses.
  // XXXbz this duplicates code in nsCSSRuleProcessor
  static const nsEventStates sPseudoClassStates[] = {
#define CSS_PSEUDO_CLASS(_name, _value)         \
    nsEventStates(),
#define CSS_STATE_PSEUDO_CLASS(_name, _value, _states)	\
    _states,
#include "nsCSSPseudoClassList.h"
#undef CSS_STATE_PSEUDO_CLASS
#undef CSS_PSEUDO_CLASS
    // Add more entries for our fake values to make sure we can't
    // index out of bounds into this array no matter what.
    nsEventStates(),
    nsEventStates()
  };
  PR_STATIC_ASSERT(NS_ARRAY_LENGTH(sPseudoClassStates) ==
                     nsCSSPseudoClasses::ePseudoClass_NotPseudoClass + 1);

  nsCOMPtr<nsIAtom> atom = do_GetAtom(aStatePseudo);
  // Our array above is long enough that indexing into it with
  // NotPseudoClass is ok.
  return sPseudoClassStates[nsCSSPseudoClasses::GetPseudoType(atom)];
}
Note that the attachment above has most of the infrastructure for locking states in general, not just the ESM-managed ones, but is pretty incomplete.
Oh crap, I'm sorry )-: I had no idea it was being worked on, and I figured you guys were busy. But thanks for looking into this bug and posting your code. Your patch brings up a good talking point.

I think devtools has come to the consensus that just changing the presentation of the element is fine (and even preferable to) changing the actual state. To that end, I've implemented that in the most unobtrusive way I could think of, not touching the element's state, but having CSSRuleProcessor check the element's "presentation state" instead.

Here's what I have so far. I'm not sure what the implications are of changing the element's "real" state bits, or whether that would yield the same behavior wrt what we want. Or if we need an API to lock the element's real bits for other reasons. But I think this patch gives devtools the behavior it wants (curious if this goes for Firebug as well?), let me know if it's going about that in the wrong way.
> I had no idea it was being worked on

Well, it wasn't on my end until I made comment 16.  ;)  I didn't know you were working on it either.

As far as the attached patch goes, it seems to be roughly equivalent to what I was doing.  I see the point about not changing mState; for things like :hover this is "safe" in that it only affects selector matching, but for other bits it might affect other behavior.  That said, the other behavior has to do with form control and image rendering and response to clicks, for the most part.  Things like whether the disabled bit is set, say.  Do we want to allow locking a form control into :disabled without giving it actual disabled behavior?

We don't need an API to lock anything for other reasons; we only need this for devtools.

I don't think we want to add the extra 8 bytes to every element to support this.  Having a boolean flag and a property storing the info we need in the rare case that we have a lock (which is what I was just starting to implement) may be better.

The last issue I see is that the attached patch doesn't allow locking an element "not in hover", say.  I thought that was a requirement, but maybe I misunderstood the end of comment 3?
(In reply to Boris Zbarsky (:bz) from comment #21)
> As far as the attached patch goes, it seems to be roughly equivalent to what
> I was doing.  I see the point about not changing mState; for things like
> :hover this is "safe" in that it only affects selector matching, but for
> other bits it might affect other behavior.  That said, the other behavior
> has to do with form control and image rendering and response to clicks, for
> the most part.  Things like whether the disabled bit is set, say.  Do we
> want to allow locking a form control into :disabled without giving it actual
> disabled behavior?

Right now we're focusing on the classes :hover, :active, :focus, and :visited. necessity for the other pseudo-classes isn't there yet, :disabled in particular will hopefully come with easier attribute editing in the devtools. So if the behavior is right for those four classes, then I think we're good.

Regardless, the use case for this is inspecting styles. You're likely to set the pseudo-class, play around with the CSS properties and check the appearance, then unset it.

> I don't think we want to add the extra 8 bytes to every element to support
> this.  Having a boolean flag and a property storing the info we need in the
> rare case that we have a lock (which is what I was just starting to
> implement) may be better.

Definitely, good call.

> The last issue I see is that the attached patch doesn't allow locking an
> element "not in hover", say.  I thought that was a requirement, but maybe I
> misunderstood the end of comment 3?

Good question. I meant "reset" not "unset", we wouldn't need to lock in the un-state. The pseudo-classes we're focusing on are easy to see in their un-state, not :hovering is easy to do naturally, same for :focus and :active. :visited is a different story, but luckily there's an un-state for it, :link. and we'll probably give that option for links as well.

Sorry for not being clear about these details.
(In reply to Heather Arthur [:harth] from comment #22)
> (In reply to Boris Zbarsky (:bz) from comment #21)
> > The last issue I see is that the attached patch doesn't allow locking an
> > element "not in hover", say.  I thought that was a requirement, but maybe I
> > misunderstood the end of comment 3?
> 
> Good question. I meant "reset" not "unset", we wouldn't need to lock in the
> un-state. The pseudo-classes we're focusing on are easy to see in their
> un-state, not :hovering is easy to do naturally, same for :focus and
> :active. :visited is a different story, but luckily there's an un-state for
> it, :link. and we'll probably give that option for links as well.

I take this back, we will need a way to set "not visited", or have logic so that locking :link/unvisited will
lock "not visited".
Regarding comment 20:
Firebug needs this feature in the same way as devtools do, i.e. we want to display the pseudo-class rules inside our Style side panel and allow the user to edit the styles. See http://code.google.com/p/fbug/issues/detail?id=3230 for more details.

Regarding comment 21 and 22:
:disabled is not a transitory state, while the other ones mentioned are. And Firebug already displays :disabled styles, if a form control is disabled. So we're fine, if the new API just covers the other mentioned pseudo-classes.

Sebastian
> :disabled is not a transitory state,

In what sense?  It's not any less transitory than :link or :visited.  Similar for things like :valid or :invalid (and getting things into :-moz-ui-invalid positively takes work).
> In what sense?
In the sense, that when I add the "disabled" attribute to the form element, I will see :disabled rules permanently inside the Style side panel.

> It's not any less transitory than :link or :visited.
Agree. Though the use case here is different. When a link is visited you have to remove all the visits from the history to reset it to its unvisited state.
So it's not just less transitory, but hard to change the state.

> Similar for things like :valid or :invalid (and getting things into
> :-moz-ui-invalid positively takes work).
Yes.

What I wanted to say is, that Firebug won't need all states.
Of course the cleanest way is to implement all possible pseudo-classes into that API, but I assume, that will also take more work. Work, that might not be needed.

Sebastian
In practice, if you have to do even one non-ESM-managed state, you might as well do all of them, imo, because they're all so similar.  The one exception is :link and :visited (well, and :-moz-any-link), which need special care because of the mutual exclusivity.
Here's what I have. Adds four methods to DOMUtils:

addPseudoClassLock(element, ":hover", [lockOff])
removePseudoClassLock(element, ":hover", [lockOff])
hasPseudoClassLock(element, ":hover", [lockOff])
clearPseudoClassLocks(element)

You can either lock the presentation state on or lock it off. This is necessary for locking an element's "unvisited" state in "off" when locking it's "visited" state in "on", if you don't do this the behavior is undefined, I think some assertions are hit at the least if you don't track it outside the API.

Locking will only affect the element's presentation and won't affect the actual state or behavior of the element.

bz, I used your getStatesForPseudoClass() verbatim, and I wonder what I should do with the attribution in the commit history.
Attachment #582385 - Attachment is obsolete: true
Attachment #591859 - Flags: review?(bzbarsky)
I don't care that much about the attribution, though if you do you can just break this up into two changesets, right?

I _do_ think someone other than me should review the getStatesForPseudoClass stuff.

For the rest, I likely won't get to it till Monday.
Whoops, one of the tests wasn't testing what it was supposed to, fixed in this patch.
Attachment #591859 - Attachment is obsolete: true
Attachment #591859 - Flags: review?(bzbarsky)
Attachment #592252 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #29)
> I _do_ think someone other than me should review the getStatesForPseudoClass
> stuff.

I could review that if you want.
Comment on attachment 592252 [details] [diff] [review]
API for locking element's style state v1, +fixed test

That'd be great!
Attachment #592252 - Flags: review?(mounir)
Comment on attachment 592252 [details] [diff] [review]
API for locking element's style state v1, +fixed test

Review of attachment 592252 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for layout/inspector/src/inDOMUtils.cpp

I had a quick look at the other files but nothing worth being called a review.

Let me know if I should have reviewed something else.

::: content/base/src/nsGenericElement.cpp
@@ +1383,5 @@
> +      if (locks) {
> +        return *locks;
> +      }
> +    }
> +    else {

nit: coding style requires:
} else {

You are doing that in a few other places.

::: layout/inspector/src/inDOMUtils.cpp
@@ +410,5 @@
> +inDOMUtils::AddPseudoClassLock(nsIDOMElement *aElement,
> +                               const nsAString &aPseudoClass,
> +                               bool aLockOff)
> +{
> +  NS_ENSURE_ARG_POINTER(aElement);

nit: I would remove that check. That could be removed from the following functions too.

@@ +431,5 @@
> +NS_IMETHODIMP
> +inDOMUtils::HasPseudoClassLock(nsIDOMElement *aElement,
> +                               const nsAString &aPseudoClass,
> +                               bool aLockOff,
> +                               bool *_retval)

nit: we don't use underscore as a prefix in Gecko AFAIK.

::: layout/inspector/tests/test_bug708874.html
@@ +90,5 @@
> +  is(DOMUtils.hasPseudoClassLock(button, ":disabled"), true,
> +     "hasPseudoClassLock is true after locking");
> +  
> +  is(window.getComputedStyle(button).color, disabledColor,
> +     ":disabled style applied after adding lock");

Could you test |mozMatchesSelector| too?
that check might be needed in other places in this file.
Attachment #592252 - Flags: review?(mounir) → review+
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #33)
> Comment on attachment 592252 [details] [diff] [review]
> API for locking element's style state v1, +fixed test
> 
> Review of attachment 592252 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me for layout/inspector/src/inDOMUtils.cpp

r=me for nsINode.h changes. I think that should cover all bz changes that have been copy-pasted to your patch, Heather.
Comment on attachment 592252 [details] [diff] [review]
API for locking element's style state v1, +fixed test

General comment: the IDL should document what aPseudoClass should be (most especially whether it should include the leading ':' or not) and what the aLockOff argument means.

>+++ b/content/base/public/Element.h
>+   * The presentation state of this element. This is the real state
>+   * of the element with any style locks applied for pseudo-class inspecting.
>+   */
>+  nsEventStates PresState() const {
>+    return (mState | PresLocks(false)) & ~PresLocks(true);

This is very performance-sensitive.  Can you inline the no-locks fast-path of PresLocks, please?  So check HasLockedStates() and return mState if false, else do the more complicated thing involving function calls?

Also, maybe call this StyleState(), no PresState(), since it affects more than just presentation (e.g. it affects querySelector calls)?

>+  /**
>+   * The presentation state locks applied to this element. Either the on
>+   * or off locks.
>+   */
>+  nsEventStates PresLocks(bool aLockOff) const;

Again, s/presentation state/style state/ (and elsewhere in the patch).  Given the implementation and the above suggestion to inline the HasLockedStates() check into StyleState(), I think it's actually better to just have two getters here: LockedOnStates() and LockedOffStates().

>+   * Add or remove a presentation state lock on the element. The lock either locks
>+   * the state on or locks it off.
>+   */
>+  void SetPresLocks(nsEventStates aStates, bool aLockOff, bool aAddLock);

I really hate the boolean arguments in that API, but short of replacing it by 4 functions I'm not sure what to do about it.  Maybe it would be better to do that, though; I don't see what names to give the two booleans that would not be confusing.  The problems come with the unlocking functions, because one can unlock on and off bits separately...  Perhaps we should simply disallow that (because, e.g. locking a single bit both on and off really doesn't make sense) and have a single unlocking function?  Then the API would be:

  LockStyleStatesOn(nsEventStates aStates);
  LockStyleStatesOff(nsEventStates aStates);
  UnlockStyleStates(nsEventStates aStates);

and LockStyleStatesOff would remove any bits in the "on" bucket that are now being locked off?

Another possibility is to have the API look like this:

  LockStyleStates(nsEventStates aLockedOnStates, nsEventStates aLockedOffStates);
  UnlockStyleStates(nsEventStates aLockedOnStates, nsEventStates aLockedOffStates);

perhaps?

>+  /**
>+   * Clear all presentation state locks on this element.
>+   */
>+  void ClearPresLocks();

ClearStyleLocks.

>+++ b/content/base/public/nsINode.h
>+    // Set if the element has some presentation states locked
>+    ElementHasLockedPresStates,

As before s/Pres/Style/ through this file.

>+++ b/content/base/src/nsGenericElement.cpp

Leaving this be for now until we agree on the API.  My only concern here is that SetPresLocks can land in a situation where it removes all the locks but leaves the "has locked states" bit set, which means that careless use of the inspector APIs could permanently slow down style matching for a node.  Probably better to not let that happen.

>+++ b/content/base/src/nsGkAtomList.h
>+GK_ATOM(lockedOnPresStates, "locked-on-pres-states")
>+GK_ATOM(lockedOffPresStates, "locked-off-pres-states")

Again, style states

>+++ b/layout/base/nsIPresShell.h
>+  virtual void ContentStateChanged(nsIDocument* aDocument,

This should be called from the Element code, not from the inspector utils, I think.

>+++ b/layout/inspector/src/inDOMUtils.cpp
>+GetStatesForPseudoClass(const nsAString& aStatePseudo)

I know I suggest this code... but the way it's written it will actually do something slightly wrong for the :moz-any-link psedo-class.  In particular, it will cause that case to set both LINK and VISITED bits.  It's probably better to check if the atom is that pseudo-class and return LINK instead of the array entry...  Or something.  That can still land you elements which have both bits set at once, which is suboptimal.  Maybe we should just throw from attempts to lock :-moz-any-link?  And for that matter :link and :visited?  If we want to allow locking those, we need to do a bit more work in terms of not allowing :link to be locked on while a link becomes :visited....



>+  nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);
>+  content->AsElement()->SetPresLocks(state, aLockOff, aAdd);

You should be able to QI to dom::Element directly.

>+  nsIDocument *ownerDoc = content->OwnerDoc();
>+  nsIPresShell *presShell = ownerDoc->GetShell();

presShell could be null.  When this code moves to the element, please don't forget to null-check it.

>+++ b/layout/style/nsCSSRuleProcessor.cpp

You need to change IsLink as well.  Otherwise, if you lock :link on for a node that's not a link you'll hit fatal aborts in the style code.

I assume you don't care about native theme styling not matching CSS styling?  If you do, you'll need to change the native theme code to also look at the style state.
Attachment #592252 - Flags: review?(bzbarsky) → review-
Oh, and in general this looks really good!  Thank you again for doing it!
(In reply to Boris Zbarsky (:bz) from comment #35)
> Then the API would be:
> 
>   LockStyleStatesOn(nsEventStates aStates);
>   LockStyleStatesOff(nsEventStates aStates);
>   UnlockStyleStates(nsEventStates aStates);
> 
> and LockStyleStatesOff would remove any bits in the "on" bucket that are now
> being locked off?
> 
> Another possibility is to have the API look like this:
> 
>   LockStyleStates(nsEventStates aLockedOnStates, nsEventStates
> aLockedOffStates);
>   UnlockStyleStates(nsEventStates aLockedOnStates, nsEventStates
> aLockedOffStates);
> 
> perhaps?

I like the latter. And with the way we're using the API right now, we'd never set an off lock without setting an on lock at the same time. We're just using the off locks to lock :link off when we lock :visited on and vice versa.

When locking a state on we could remove any off lock for that state, and when locking a state off remove any on locks for the state.

> >+++ b/layout/inspector/src/inDOMUtils.cpp
> >+GetStatesForPseudoClass(const nsAString& aStatePseudo)
> 
> I know I suggest this code... but the way it's written it will actually do
> something slightly wrong for the :moz-any-link psedo-class.  In particular,
> it will cause that case to set both LINK and VISITED bits.  It's probably
> better to check if the atom is that pseudo-class and return LINK instead of
> the array entry...  Or something.  That can still land you elements which
> have both bits set at once, which is suboptimal.  Maybe we should just throw
> from attempts to lock :-moz-any-link?  And for that matter :link and
> :visited?  If we want to allow locking those, we need to do a bit more work
> in terms of not allowing :link to be locked on while a link becomes
> :visited....
> 

I really think the pseudo-classes we're targeting are the most important thing for us right now (:hover, :active, :focus, :visited, :link), so I'd be fine with throwing on :moz-any-link or any other class we don't need.

As for :visited and :link, we want to be able to make any link look visited (match :visited) and make it look unvisited (match :link). Right now I have this logic to lock :visited (and at the same time unlock :link and lock :link off) in JS-land. But given that it could get into a nasty undefined state, we could handle this logic inside DOMUtils.

Are there any other complimentary pseudo-classes like these? It looks like :enabled/:disabled, :valid/:invalid also. At the same time, we don't need these right now. I don't know how future-proof we want to make this API.

> I assume you don't care about native theme styling not matching CSS styling?
> If you do, you'll need to change the native theme code to also look at the
> style state.

I'm not sure, what do you mean by native theme styling? How often does it happen?
Target Milestone: --- → mozilla12
> But given that it could get into a nasty undefined state, we could handle this logic
> inside DOMUtils.

I think we should consider handling this logic in Element itself.  Otherwise it's possible to break some pretty core rendering engine invariants.

> Are there any other complimentary pseudo-classes like these? 

Yes, but I don't think anything internally depends on them not being set at the same time....

> I'm not sure, what do you mean by native theme styling? How often does it happen?

I mean the code that makes form controls (buttons, comboboxes, etc) look like native OS controls.  It happens for pretty much all form controls.  The native theme look depends on things like hover/focus state.
(In reply to Boris Zbarsky (:bz) from comment #38)
> > But given that it could get into a nasty undefined state, we could handle this logic
> > inside DOMUtils.
> 
> I think we should consider handling this logic in Element itself.  Otherwise
> it's possible to break some pretty core rendering engine invariants.

Makes sense. If we handled the visited/link logic in Element then devtools wouldn't need an API for locking states off. In fact, if we're just checking visited/link then maybe we wouldn't even need a lockedOffStates property? When getting the StyleState(), if there are locks and visited is locked on, then we could mask out unvisited before returning the style state. A thought.

> > I'm not sure, what do you mean by native theme styling? How often does it happen?
> 
> I mean the code that makes form controls (buttons, comboboxes, etc) look
> like native OS controls.  It happens for pretty much all form controls.  The
> native theme look depends on things like hover/focus state.

Ah. I guess this would be nice. I'll look into it.
If we can skip locking states off just by having Element handle :link and :visited correctly internally, then that sounds like a huge win.  It would simplify the API a good bit!  Your proposal for how to implement it sounds exactly right.
(In reply to Boris Zbarsky (:bz) from comment #35)
> >+++ b/layout/style/nsCSSRuleProcessor.cpp
> 
> You need to change IsLink as well.  Otherwise, if you lock :link on for a
> node that's not a link you'll hit fatal aborts in the style code.
>

Confused about this, you mean change it to check the StyleState() as well?
How should the DOMUtils API handle null element arguments?
Throwing, like your patch did, makes sense to me.
Patch addressing comments. Removes the whole "off" lock API and just toggles :visited when :link is locked and vice versa.
Attachment #592252 - Attachment is obsolete: true
Attachment #594086 - Flags: review?(bzbarsky)
Comment on attachment 594086 [details] [diff] [review]
API for locking element's style state v2

>+++ b/content/base/public/Element.h
>+  nsEventStates StyleState() const;

We really do want to inline the fast path here.  How about this:

  nsEventStates StyleState() const {
    if (asLockedStyleStates()) {
      return mState;
    }

    return ComputeStyleState();
  }

where ComputeStyleState (or some such name) is private and implemented out of line?

>+++ b/content/base/public/nsINode.h

This needs merging to another bit that got added recently, but that shouldn't be a big deal.

>+++ b/content/base/src/nsGenericElement.cpp
>+Element::NotifyStyleStateChange(nsEventStates aStates)
>+  nsIDocument* doc = GetCurrentDoc();

doc can be null.  Needs a null-check.

>+    nsIContent* content = this;
>+    presShell->ContentStateChanged(doc, content, aStates);

Why do you need |content|?  Just doing this:

  presShell->ContentStateChanged(doc, this, aStates);

should work.

Also, you need an autoscriptblocker here, just like NotifyStateChange.

>+Element::UnlockStyleStates(nsEventStates aStates)
>+  nsEventStates *locks = new nsEventStates(LockedStyleStates());
>+
>+  *locks &= ~aStates;
>+
>+  if ((*locks).IsEmpty()) {

  if (locks->IsEmpty()) {

>+    DeleteProperty(nsGkAtoms::lockedStyleStates);
>+    ClearHasLockedStyleStates();

You also need to delete |locks| in that case, right?  Otherwise it'll leak.

>+++ b/content/base/src/nsGkAtomList.h
>+GK_ATOM(lockedStyleStates, "locked-style-states")

This should probably go in the "Content property names" section of the file (around line 1845 in my version right now).

>+++ b/layout/base/nsIPresShell.h

Need to change the presshell IID.

>+++ b/layout/inspector/public/inIDOMUtils.idl
>+  // selector string, e.g. ":hover".":moz-any-link" and non-ESM pseudo-classes are ignored.

Space after the '.' after ":hover", please.  And it's ":-moz-any-link" (here and in the C++ comments).

Also, the non-ESM thing doesn't make sense (e.g. :disabled is not ignored, but has nothing to do with the ESM).  Do you mean non-event-state pseudo-classes?

>+++ b/layout/inspector/src/inDOMUtils.cpp
>+inDOMUtils::HasPseudoClassLock(nsIDOMElement *aElement,
>+                               const nsAString &aPseudoClass,
>+                               bool *_retval)
...
>+  if (state.IsEmpty()) {
>+    return NS_OK;

Need to set *_retval to something there.  Probably to false.

>+++ b/layout/inspector/tests/test_bug708874.html
>+netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

We're trying to get rid of that.  Please either add SpecialPowers APIs or make this a chrome test?

r=me with the above issues fixed.  This looks great!  Still the open question about native theme code.  I _think_ just changing the callsite in nsNativeTheme::GetContentState would be enough to handle that.
Attachment #594086 - Flags: review?(bzbarsky) → review+
Thanks for the reviews bz. Uploading the patch updated to your comments.
(In reply to Boris Zbarsky (:bz) from comment #46)
> Also, the non-ESM thing doesn't make sense (e.g. :disabled is not ignored,
> but has nothing to do with the ESM).  Do you mean non-event-state
> pseudo-classes?

Changed comment to say "non-event-state", I mean all those structural pseudo-classes that don't have event state bits.

> Still the open question about native theme code.  I _think_ just changing the callsite in
> nsNativeTheme::GetContentState would be enough to handle that.

Thanks for the tip. Unfortunately that wasn't enough. PresShell->ContentStateChanged() doesn't seem to trigger an update of the native theme layout. Another bug, I think. It's not a huge priority.
Thanks, that last attachment looks great.  Can you push it to inbound yourself, or do you want me to?

Odd that ContentStateChanged doesn't update the native theme stuff; it should be calling into WidgetStateChanged from inside nsCSSFrameConstructor::ContentStateChanged...  I agree that a followup bug for that is fine.
(In reply to Boris Zbarsky (:bz) from comment #49)
> Thanks, that last attachment looks great.  Can you push it to inbound
> yourself, or do you want me to?

I'll push it.

> Odd that ContentStateChanged doesn't update the native theme stuff; it
> should be calling into WidgetStateChanged from inside
> nsCSSFrameConstructor::ContentStateChanged...  I agree that a followup bug
> for that is fine.

Well, not sure if it's exactly that, but after locking :active, :active styles don't show up until it's moused over. Then disappear when it's moused out oddly. Will file.
Without locking :active, just with normal mouse clicking, do they also disappear on mouseout?  I think some OS active styles are actually tied to :hover:active.
(In reply to Boris Zbarsky (:bz) from comment #51)
> Without locking :active, just with normal mouse clicking, do they also
> disappear on mouseout?  I think some OS active styles are actually tied to
> :hover:active.

Ah, that's totally what's going on. Well crap, I just pushed, but I'll file that bug.

http://hg.mozilla.org/integration/mozilla-inbound/rev/e4df2fc85668

Man, thanks for all the time and patience bz.
No problem.  Thanks for diving into this stuff!
https://hg.mozilla.org/mozilla-central/rev/e4df2fc85668
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla12 → mozilla13
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: