Closed Bug 1221297 Opened 4 years ago Closed 4 years ago

Keyframe syntax is showing up in rule view as part of a rule

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

VERIFIED FIXED
Firefox 45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: bgrins, Assigned: tromey)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached image keyframe-ruleview.png
Open http://devtoolschallenger.com/
Inspect the #creature_orange-roughy1 element
Navigate rule view to gentlyHovering

100% {
  ☑  100% { -webkit-transform: translate(-50%, 15%);
  ☑  transform: translate(-50%, 15%);
}

I'm not expecting to see the '100% {' part on the second line (see screenshot).

This appears to be coming from CSS like:

@keyframes gentlyHovering {
  100% {
    -webkit-transform: translate(-50%, 15%);
            transform: translate(-50%, 15%);
  /*! } */
}
}
QA Contact: ttromey
Assignee: nobody → ttromey
QA Contact: ttromey
I couldn't reproduce this, maybe because for me the rule doesn't look like that.
From main.css:1101:

@keyframes gentlyHovering {
  100% {
    -webkit-transform: translateY(15%);
            transform: translateY(15%);
  }
}

If I edit in a comment like /*! } */, I still don't see the reported problem.

Did you do some editing or anything before seeing the problem?
Flags: needinfo?(bgrinstead)
fwiw, I'm not reproducing this either (on windows, linux and mac).
I also wanted to note that the "/*!" style of comment is what is used by
the rule view when you comment out a property; which made me think maybe
there was some previous action.
I must have triggered it earlier in the tutorial somehow.  I do remember that I couldn't find the 'filter' rule on the #creature_orange-roughy1 element.  I must have closed and reopened the toolbox a couple of times, and may have even reloaded the page.  Eventually I got the filter to show up and edited it, but then noticed the issue with genylyHovering keyframe.

I just went through this whole process again, and the filter showed up immedately but I scrolled down to gentlyHovering and noticed that the -webkit- part in it was gone and the line number moved to inline:-1.  So I wonder if one of the previous animation tutorial things is changing that.
Flags: needinfo?(bgrinstead)
Yeah I can reproduce at least this part:

Go to http://devtoolschallenger.com/
Follow prompts on page (unpause animation on fish 1, edit cubic bezier on nautilus, step through animation on squid).  Note I open the toolbox first by inspecting the first fish and I leave it open the whole time, scrolling at a normal speed through the page as I go).
Inspect the #creature_orange-roughy1 element
Scroll down in rule view to keyframes

Expected:

100% {                                     main.css:1102
    -webkit-transform: translateY(15%);
    transform: translateY(15%);
}

Actual:

100% {                                     inline:-1
    transform: translateY(15%);
}
One oddity is that I can't see the keyframe unless the orange roughy element
is actually visible.  That is, if I find it in the markup view and examine it,
I don't see "gentlyHovering" in the rule view at all.
However, if I then scroll it into view, click some other element, and then click
back, the keyframe rule appears.

I will file a separate bug about this tomorrow.  (I doubt this is caused by
as-authored.)


Second, I was able to reproduce the "inline:-1" problem by working through the
page in order.  However, when I did things in a different order, it didn't reproduce.
That's funny.

I don't yet have a theory for what is causing this bug.
I can reproduce by editing the nautilus and then the roughy.
However if I view the roughy first in the inspector, then the problem
does not occur.
Fun and mysterious!

The other edits don't matter.

My hypothesis right now is that the code we added to parseStyleSheet to
try to preserve line numbers is not affecting the rules in the keyframe somehow.
Ok, I don't yet know why the location changes to "inline:-1", but with
a much simpler animation example, I can see that key frame line numbers
don't update when I edit the style sheet in the style editor -- but 
the line numbers of other rules *do* update.  So this is suggestive,
and certainly a bug in its own right.
(In reply to Tom Tromey :tromey from comment #6)
> One oddity is that I can't see the keyframe unless the orange roughy element
> is actually visible.  That is, if I find it in the markup view and examine
> it,
> I don't see "gentlyHovering" in the rule view at all.
> However, if I then scroll it into view, click some other element, and then
> click
> back, the keyframe rule appears.
> 
> I will file a separate bug about this tomorrow.  (I doubt this is caused by
> as-authored.)
That's because js on the page listens to the scroll event to know when things are in view and only animates them then. When you scroll down and when #d1600 becomes visible in the viewport, it gets the 'in-view' class added, and that's what starts the animation.
There are a couple of bugs.

PageStyleActor has a CssLogic instances that caches the keyframe rules.
I think we have to clear this in _styleApplied.

Also, _computeRuleIndex gets -1 for a keyframe rule.  This happens because
it has no concept of nested rules, and so it doesn't recognize that the
rule is nested in @keyframes.  My current idea here is to turn _ruleIndex
into a vector.
Those help my reduced test case but don't actually fix the original bug.  Sad!

However I think this may be related to the original bug; seen with an
unmodified debug build:

console.error: 
  Got an addition of an actor we didn't know about: server1.conn0.child1/domnode219
The final problem turned out to be a cache invalidation bug.  My
initial attempt wasn't clearing the keyframe cache on
UPDATE_PRESERVING_RULES updates -- but it has to, to avoid holding on
to orphaned key frame rules.
Attachment #8683806 - Flags: review?(pbrosset)
Comment on attachment 8683806 [details] [diff] [review]
update line numbers for nested rules in rule view

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

r=me with following questions either addressed or answered (you know much more than me about this part of the code now).

::: devtools/client/styleinspector/test/browser_ruleview_keyframeLineNumbers.js
@@ +19,5 @@
> +
> +  let editor = yield focusEditableField(view, elementRuleEditor.closeBrace);
> +  editor.input.value = "font-size";
> +  let onValueFocus = once(elementRuleEditor.element, "focus", true);
> +  let onModifications = elementRuleEditor.rule._applyingModifications;

I think you should be able to use 'view.once("ruleview-changed")' instead here (same further below after the value has been entered).

::: devtools/server/actors/styles.js
@@ +1281,5 @@
>     */
>    _computeRuleIndex: function() {
>      let rule = this.rawRule;
> +    let result = [];
> +    while (rule) {

really nit: I'd like some more empty lines in this function. One before this while statement. And a couple around the for statement below.

@@ +1528,5 @@
>          }
>        }
>      }
>  
> +    return this._getRuleFromIndex(parentStyleSheet);

Can there be cases where this._ruleIndex is null here?
It may be set to null in _computeRuleIndex and there's no null check in _getRuleFromIndex or here.
Attachment #8683806 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #13)

> > +    return this._getRuleFromIndex(parentStyleSheet);
> 
> Can there be cases where this._ruleIndex is null here?
> It may be set to null in _computeRuleIndex and there's no null check in
> _getRuleFromIndex or here.

No, it shouldn't be possible.

initialize calls _computeRuleIndex for a style or keyframe rule.
_computeRuleIndex can only fail for orphaned rules, which should not
be possible.
Finally, I think _addNewSelector should only be called on style rules.
Attachment #8683806 - Attachment is obsolete: true
Attachment #8683882 - Flags: review+
The test was either failing on try or locally, depending on how I
altered it.  Hopefully this variant is ok; debugging locally I found a
race when selecting a new node in the inspector, and this version
waits for the rule view to refresh before moving on.
Attachment #8683882 - Attachment is obsolete: true
Attachment #8684279 - Flags: review+
Use createNewRuleViewProperty rather than trying to create the new
property manually.  At least locally this fixes the race.
Attachment #8684279 - Attachment is obsolete: true
Attachment #8684337 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0048cb0c2612
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Comment on attachment 8684337 [details] [diff] [review]
update line numbers for nested rules in rule view

Approval Request Comment
[Feature/regressing bug #]: 984880
[User impact if declined]:
In some cases (in particular when editing keyframes), a change made in
the rule view will cause other rules in the rule view to appear to come
from location "inline:-1".
In particular this problem can be seen while working on Devtools Challenger.
[Describe test coverage new/current, TreeHerder]:
A new test covers the new behavior.
[Risks and why]: 
Despite the apparent size of the change, this is relatively low risk.
The change to _computeRuleIndex is careful to bail out in an equivalent
(to the status quo ante) way if something truly unexpected is seen.
The most likely possible failure mode is an exception which can be fixed
by selecting some other element in the inspector, then going back to the
element of interest.
[String/UUID change made/needed]: No.
Attachment #8684337 - Flags: approval-mozilla-aurora?
Brian, could you please verify this fix works as expected and that there are no unexpected fallouts? This would give me more confidence in approving this for uplift to Aurora. Thanks!
Flags: needinfo?(bgrinstead)
(In reply to Ritu Kothari (:ritu) from comment #25)
> Brian, could you please verify this fix works as expected and that there are
> no unexpected fallouts? This would give me more confidence in approving this
> for uplift to Aurora. Thanks!

I've confirmed my original issue (following steps in Comment 5) is resolved in latest builds.
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #26)
> (In reply to Ritu Kothari (:ritu) from comment #25)
> > Brian, could you please verify this fix works as expected and that there are
> > no unexpected fallouts? This would give me more confidence in approving this
> > for uplift to Aurora. Thanks!
> 
> I've confirmed my original issue (following steps in Comment 5) is resolved
> in latest builds.

Thanks a lot! I'll approve Aurora uplift given that this was verified.
Comment on attachment 8684337 [details] [diff] [review]
update line numbers for nested rules in rule view

The fix was verified on Nightly, it seems safe to uplift to Aurora44.
Attachment #8684337 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.