Closed Bug 1590031 Opened 5 years ago Closed 2 years ago

[Track Changes] Every keystroke shown as a change

Categories

(DevTools :: Inspector: Changes, defect, P2)

defect

Tracking

(firefox103 fixed)

RESOLVED FIXED
103 Branch
Tracking Status
firefox103 --- fixed

People

(Reporter: birtles, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Whiteboard: [dt-q])

Attachments

(4 files)

Attached image image.png

I don't have a concrete STR but hopefully the screenshot gives some hints.

I've seen this a few times where the Changes panel shows every keystroke I've entered. I believe the initial value I entered was grid-template-rows: 1fr 1fr. When I tried to turn that into repeat(2, 1fr) things started to go haywire.

(Interestingly I never entered the \ escape characters. DevTools decided to enter those for me and I had to delete them. Not sure why. Also, when I entered the extra closing ) after adding minmax, DevTools swallowed it because the next character was \ so I needed to enter it twice.)

(And yet another totally unrelated observation, the property would be marked as invalid when I had minmax(1fr, auto) since apparently that's not valid. It would be nice if DevTools could tell me why it's not valid.)

There are quite a few cases where this happens (See Bug 1529419).

Best guess so far is that the string is escaped before being sent from the client to to the server. Something gets lost in translation and the Changes panel believes the escaped string is new. This gets compounded over time.

I will prioritize looking into this. There are other use cases which cause this, like typing a font family value with quotes.

Assignee: nobody → rcaliman
Priority: -- → P2
See Also: → 1529419
Status: NEW → ASSIGNED

I just found other STRs. All keystrokes are tracked when using quotes, as in my example where I tried to enter font-family: 'zilla slab'.

Whiteboard: [dt-q]
Attached video 77Lq3sQW6w.mp4

Here's a video of the bug in action.

All keystrokes are tracked when using quotes

It also seems to be the case that this bugs appears with parentheses in the property value.

Severity: normal → S2

Hi Razvan, can you confirm here if you're targeting 79 for this S2?

Flags: needinfo?(rcaliman)

Hi Rachel, I'll put it on my list but I can't guarantee yet if it will make it in 79.
I'll have a better answer after I investigate it a bit.

Flags: needinfo?(rcaliman)

Razvan, I don't think this is S2, so changing to S3, but keeping in P2 for DevTools
But, let me know if you think otherwise, thanks!

Honza

Severity: S2 → S3

Agreed, S3 P2 sounds good.

Assignee: rcaliman → nobody
Status: ASSIGNED → NEW
Summary: Every keystroke shown as a change → [Track Changes] Every keystroke shown as a change
See Also: → 1764533

Should check the status of this issue.

Flags: needinfo?(jdescottes)

As far as I can see, the issue is that the server will update its known list of "declarations" based on what has been effectively applied. See https://searchfox.org/mozilla-central/source/devtools/server/actors/style-rule.js#462-464

So for instance, if you type font-family: 'zill (no closing quote after "zill"), the ChangesView frontend thinks that for the property name font-family, we have a value 'zill. But for the server, the declaration was autocorrected and the value actually becomes 'zill' (closing quote here).

Basically anytime there is a disconnect between the authored value and the applied value, the Changes view will lose track of things.

I think a way forward is around the logDeclarationChange method (https://searchfox.org/mozilla-central/rev/973000acec0cbf7211e0fad89ca00c352aeb8384/devtools/server/actors/style-rule.js#916). This method is called before we apply the changes and it will create the data consumed by the changes view. The issue is that it runs before we applied the actual CSS change so it can't know for sure if the user input was applied as is or was modified. We should change it so that it emits the applied value, and not just the authored value. This might not be trivial because it still need to run partly before we apply the change, in order to the previous values. Maybe we need to split it in a 3 step process:

  • collect the existing values
  • apply the change
  • emit the event for the Changes view
Flags: needinfo?(jdescottes)

I quickly tried to prototype this and there is an additional issue. The list of declarations is updated on the server side in the form method of the style-rule actor. This makes it a bit hard to rely on. The form method is called by the framework whenever the actor needs to be returned to the client.

That means that when a server-side method ends with return this, the DevTools framework will serialize this actor back to the frontend and to do that it will call the form method, which will update the declarations. So in order for my suggestion to work, not only do we need to split the creation of the "changes", but the last step also needs to happen after form() was called. To test it out quickly I am using a setTimeout but that's really not something we could land.

It seems to fix the issue but for a real fix, we should find a better way to handle this data on the server side. Not relying on form() would be great, or maybe we could just queue "changes" that should be notified, and emit them all when declarations are regenerated (a bit similar to what we do for mutations)

My patch is at https://treeherder.mozilla.org/jobs?repo=try&revision=abfcbe97a462e7f17846ee092c7a2f2ea296ad8d, let's see if try complains with this already.

Edit: one minor failure purely related to the usage of timeout in browser_resources_css_changes.js , otherwise try seems fine

I checked that all the issues which consistently reproduce from this bug and associated duplicates are at least fixed with my proposal. Again, we'd need to find something else than a setTimeout based solution here, but at least this confirms a lot of issues were indeed from the same root problem.

There are other scenarios (eg Bug 1772361) where it's not clear why the Changes view would start accumulating changes. I can imagine it being related to the same issue, but I guess we'll fix this first and will see if we still have reports afterwards.

The severity field for this bug is relatively low, S3. However, the bug has 4 duplicates.
:jdescottes, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jdescottes)

I think it has lots of duplicates because it is a long standing issue. However annoying, it's not really a critical feature of DevTools, so S3 is fine here.
Nevertheless I will try to cleanup and land my fix.

Flags: needinfo?(jdescottes)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED

Delay computing the CSS changes until the declarations have been updated to reflect the values understood by the engine.
This avoids trusting authored values for CSS changes, and fixes all scenarios where users typed an opening character (single quote, double quote etc...) or an escape
character.

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de8b150f3494
[devtools] Compute CSS changes based on actual values applied by the engine r=nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch
Blocks: 1783893
See Also: → 1798774
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: