Closed Bug 1323618 Opened 8 years ago Closed 7 years ago

Allow locking off of psuedo-classes through inIDOMUtils

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(1 file)

Bug 1323618 - Allow locking off of psuedo-classes through inIDOMUtils.
58 bytes, text/x-review-board-request
heycam
: review+
Details 
For bug 910022, we need to calculate the computedStyle of each option, but for the option that is selected we get back the computedStyle of 'option:selected'. This isn't helpful because it causes our code to think that the selected option should *always* look selected, even if it is not the active menuitem.

We would like the ability to temporarily remove the :selected pseudo-class to calculate the style.
I haven't looked at why the current code is not working, but:

I think we don't want to store this additional field on EventStates itself.  It will, as you pointed out, mean that we are always taking up memory for this field even when we haven't done any event state locking.  And I think it confuses the use of an EventStates object outside of this locking context.

Instead, we should have the additional field in the nsGkAtoms::lockedStyleStates property value.  This would be a struct of two EventStates values -- mLocks and mValues, for example.  In LockStyleStates we would need to look up the existing property (if it is set, or else create a new one) and merge in the new state lock request by setting the corresponding bit in mLocks and then setting the corresponding bit in mValues to the aEnabled argument value.  StyleStateFromLocks would then mask off all of the currently locked states, and OR in the corresponding bits from mValues.
Comment on attachment 8818803 [details]
Bug 1323618 - Allow locking off of psuedo-classes through inIDOMUtils.

https://reviewboard.mozilla.org/r/98744/#review99062
Attachment #8818803 - Flags: review?(cam)
Comment on attachment 8818803 [details]
Bug 1323618 - Allow locking off of psuedo-classes through inIDOMUtils.

https://reviewboard.mozilla.org/r/98744/#review99284

This is looking good, though I'd like to see another revision of the patch.


Can you please add some sub-tests to devtools/server/tests/mochitest/test_inspector-pseudoclass-lock.html that we do the right then when using this to lock states off?

::: dom/base/Element.h:247
(Diff revision 3)
> +  // xxxjaws maybe use BinaryEventStates as name?
> +  struct EventStatesLocksAndValues {

How about StyleStateLocks?  That term seems consistent with the comments on the functions below.

::: dom/base/Element.h:249
(Diff revision 3)
> +    EventStates mLocks;
> +    EventStates mValues;

Please add a comment above these two members (or one comment above the struct) explaining how they are used, especially given that there is no encapsulation and the callers need to combine mLocks and mValues.

::: dom/base/Element.h:260
(Diff revision 3)
>     */
> -  EventStates LockedStyleStates() const;
> +  EventStatesLocksAndValues LockedStyleStates() const;
>  
>    /**
>     * Add a style state lock on this element.
> +   * If aEnabled = false, the negation of aStates is locked.

I still think this comment would read better as something like:

  aEnabled is the value to lock the given state bits to.

::: dom/base/Element.cpp:351
(Diff revision 3)
> +  EventStates locks = locksAndValues.mLocks;
>    EventStates state = mState | locks;
>  
> +  // States are only used for styling if they are marked as enabled.
> +  state &= locksAndValues.mValues;

Is this the semantics we want?  I think this will turn off any bits that are unlocked, but instead we should keep their original mState values.  So I believe instead we want something like this:

  EventStates state = (mState & ~locks) |
                      (locks & locksAndValues.mValues);

i.e., take mState, clear the states that are locked, and then copy those locked state values from mValues.

::: dom/base/Element.cpp:357
(Diff revision 3)
>    if (locks.HasState(NS_EVENT_STATE_VISITED)) {
>      return state & ~NS_EVENT_STATE_UNVISITED;
>    }
>    if (locks.HasState(NS_EVENT_STATE_UNVISITED)) {
>      return state & ~NS_EVENT_STATE_VISITED;
>    }

I think these need to be changed, now that a bit present in |locks| doesn't mean that we will be forcing that bit on.  Instead, I think we can just call HasState on state rather than locks.  (We should never have the situation where mState contains both VISITED and UNVISITED.)

::: layout/inspector/inDOMUtils.cpp:1305
(Diff revision 3)
>    }
>  
>    nsCOMPtr<mozilla::dom::Element> element = do_QueryInterface(aElement);
>    NS_ENSURE_ARG_POINTER(element);
>  
> -  EventStates locks = element->LockedStyleStates();
> +  EventStates locks = element->LockedStyleStates().mLocks;

I guess this makes sense for a function named hasPseudoClassLock (since we are returning whether a given state bit is locked, not what the locked value is), but I wonder if there are any users of this function which then need updating.  Maybe not, but have a check.

::: layout/inspector/inIDOMUtils.idl:188
(Diff revision 3)
>    void getCSSPseudoElementNames([optional] out unsigned long aCount,
>                                  [retval, array, size_is(aCount)] out wstring aNames);
>  
>    // pseudo-class style locking methods. aPseudoClass must be a valid pseudo-class
>    // selector string, e.g. ":hover". ":any-link" and non-event-state
>    // pseudo-classes are ignored.

Please document the new argument here.
Attachment #8818803 - Flags: review?(cam)
Comment on attachment 8818803 [details]
Bug 1323618 - Allow locking off of psuedo-classes through inIDOMUtils.

https://reviewboard.mozilla.org/r/98744/#review99284

> I guess this makes sense for a function named hasPseudoClassLock (since we are returning whether a given state bit is locked, not what the locked value is), but I wonder if there are any users of this function which then need updating.  Maybe not, but have a check.

The current uses shouldn't need updating because they don't use this new optional argument nor provide an interface for letting users set this extra bit.
Comment on attachment 8818803 [details]
Bug 1323618 - Allow locking off of psuedo-classes through inIDOMUtils.

https://reviewboard.mozilla.org/r/98744/#review100612

r=me with these comments addresesd, thanks!

::: devtools/server/tests/mochitest/test_inspector-pseudoclass-lock.html:82
(Diff revision 5)
> +    // XXXjaws, I'm not sure why but .matches(pseudo) is resulting false for all test cases.
> +    // Since it is always returning false, the todo_is uses `true` for the expectation
> +    // because we don't want this to have an unexpected-pass when `enabled == false`.

Hmm, looks like there are some special cases in selector matching with :hover and :active for quirks mode documents, and inspector-traversal-data.html is in quirks mode.  Can you see if adding <!DOCTYPE html> helps there, and if not, file a followup bug?

::: dom/base/Element.cpp:355
(Diff revision 5)
> +  // XXXjaws: Should we be doing the same for
> +  // ENABLED/DISABLED, VALID/INVALID, INRANGE/OUTOFRANGE,
> +  // READONLY/READWRITE, etc?

Maybe, but I am not sure.  NS_EVENT_STATE_UNVISITED is different because it is set when :visited does not apply, whereas e.g. ENABLED/DISABLED have two corresponding pseudo-classes, :enabled and :disabled.

There may be some reasons to do with how Gecko computes visited and unvisited styles that needs these two separate bits, which doesn't apply to the other pseudo-classes you mention.  Now that have the ability to lock certain states off, devtools could ensure that we don't get into strange :enabled:disabled matching states from its end, rather than managing it in here.

::: dom/base/Element.cpp:412
(Diff revision 5)
>    if (aStates.HasState(NS_EVENT_STATE_UNVISITED)) {
> -    *locks &= ~NS_EVENT_STATE_VISITED;
> +    locks->mLocks &= ~NS_EVENT_STATE_VISITED;
>    }
>  
>    SetProperty(nsGkAtoms::lockedStyleStates, locks,
>                nsINode::DeleteProperty<EventStates>);

This should be DeleteProperty<StyleStateLocks> (although I wouldn't be surprised if the right thing happened even if we get the type wrong).

::: dom/base/Element.cpp:432
(Diff revision 5)
>      ClearHasLockedStyleStates();
>      delete locks;
>    }
>    else {
>      SetProperty(nsGkAtoms::lockedStyleStates, locks,
>                  nsINode::DeleteProperty<EventStates>);

Here too.
Attachment #8818803 - Flags: review?(cam) → review+
Sorry for the delay here. I made the changes you requested and switching inspector-traversal-data.html out of quirks mode fixed the .matches() issue.

However, locking the pseudo-classes to an 'off' state is not working. The pseudo class is still being styled as though it is 'on'. I've tried looking through searchfox as to how the result of LockedStyleStates() gets used in the application of styles (particularly nsCSSRuleProcessor's SelectorMatches() function) but can't find my way around the code too well. Can you help me out here?
Flags: needinfo?(cam)
The attached test is failing with the following conditions:
142 INFO TEST-UNEXPECTED-FAIL | devtools/server/tests/mochitest/test_inspector-pseudoclass-lock.html | Target should match pseudoclass, ':hover', if enabled (with .matches()) - got true, expected false
143 INFO TEST-UNEXPECTED-FAIL | devtools/server/tests/mochitest/test_inspector-pseudoclass-lock.html | Target should match pseudoclass, ':hover', if enabled (with outline check) - got true, expected false
144 INFO TEST-UNEXPECTED-FAIL | devtools/server/tests/mochitest/test_inspector-pseudoclass-lock.html | Style should not be applied for pseudoclass, ':hover', when disabled - didn't expect "rgb(255, 255, 0)", but got it
I think the boolean argment passed to gWalker.addPseudoClassLock isn't getting passed through down to inDOMWindowUtils.addPseudoClassLock.

Actually, it looks like the walker's addPseudoClassLock already has a third argument, so maybe you need to add a property to that option argument, and convert that into the boolean in the ultimate DOMWindowUtils.addPseudoClassLock call:

http://searchfox.org/mozilla-central/source/devtools/server/actors/inspector.js#1762
Flags: needinfo?(cam)
Yes, you were so right. Thank you for looking at this again Cameron, it was right under my nose all along and I was looking too deep in the stack. Thanks for your help, all tests are now passing locally for me.
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6c4807171f00
Allow locking off of psuedo-classes through inIDOMUtils. r=heycam
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc26d8805838
Allow locking off of psuedo-classes through inIDOMUtils. r=heycam
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/55e870f897ff
Backed out changeset 6c4807171f00 on jaws' request. r=backout
https://hg.mozilla.org/mozilla-central/rev/fc26d8805838
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: