Closed Bug 1659498 Opened 4 years ago Closed 10 months ago

Enable the CSS compatibility tooltip in Nightly

Categories

(DevTools :: Inspector: Rules, task, P3)

task

Tracking

(firefox115 fixed)

RESOLVED FIXED
115 Branch
Tracking Status
firefox115 --- fixed

People

(Reporter: mtigley, Assigned: nchevobbe)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files)

Now that the CSS compatibility tooltip is available behind the devtools.inspector.ruleview.inline-compatibility-warning.enabled pref, we should think about having the feature enabled in Firefox Nightly by default at some point.

Perhaps we can enable the feature in Nightly 82? What do you think, Daisuke and Prateek?

Flags: needinfo?(daisuke)
Severity: -- → S3
Priority: -- → P3

Thank you very much for filing this!
I don't see any problem to enable this feature.

Flags: needinfo?(daisuke)

This patch enables the experimental CSS compatibility tooltip in
inspector's rules panel to test for stability.

Assignee: nobody → lelouch.cpp
Status: NEW → ASSIGNED
Attachment #9186807 - Attachment description: Bug 1659498 - Enable compatibility tooltip by default in Inspector's rules panel r=mtigley,daisuke → Bug 1659498 - Enable CSS Compatibility tooltip in Nightly r=mtigley,daisuke
Pushed by mtigley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22812838c774
Enable CSS Compatibility tooltip in Nightly r=mtigley,daisuke

Backed out for multiple devtools failures due to compatibility issues.

backout: https://hg.mozilla.org/integration/autoland/rev/7c5bd84d0851a0cf5aaededce8e9a701a28cd009

push: https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=devtools&revision=22812838c774a072efb9956e2fa98d3f6022a7d1

failure logs:

Flags: needinfo?(lelouch.cpp)
Flags: needinfo?(lelouch.cpp) → needinfo?(mtigley)

Julien provided me a few tips on what we can do to solve these test failures. The main issue here is that selecting an element will trigger calls to getCompatibility / getCSSDeclarationBlockIssues and we're not waiting for them to complete before shutting down.

Part of the problem is that the rule view calls updatePropertyCompatibilityIndicator (an async function) in updatePropertyState (which is synchronous): https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/views/text-property-editor.js#786-788. One approach that could help this is to initialize the CompatibilityFront upfront, when the InspectorFront is initialized: https://searchfox.org/mozilla-central/source/devtools/client/fronts/inspector.js#47-50.

The next issue is solving the failures involving getCSSDeclarationBlockIssues, which needs to call the actor everytime. Julien has suggested the following approaches:

  • Modify the inspector opening sequence to wait for an event emitted at the of updatePropertyCompatibilityIndicator

  • Use safeAsyncMethod to silence these exceptions

The second approach seems to have a few errors still. This might be due to an issue with async fronts (i.e: fronts with a async initialize methods) when they are retrieved via protocol.js instead of getFront. This issue has been documented in Bug 1677506.

Flags: needinfo?(mtigley)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:lelouch.cpp, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(lelouch.cpp)

I unfortunately didn't have time to experiment with what I mentioned in comment 5 before going on PTO. I'll move the needinfo to myself to put this on my radar this week.

Flags: needinfo?(lelouch.cpp) → needinfo?(mtigley)

Now that https://bugzilla.mozilla.org/show_bug.cgi?id=1677660 landed, the compatibility front should initialize synchronously, and I think it can be worth trying to land this again.

Flags: needinfo?(mtigley)

I have pushed the patch to try: https://treeherder.mozilla.org/jobs?repo=try&revision=a0b22199f8c6250a8e0ac0fcef8f6e34389d58c0
It will need to be rebased on phabricator before landing though. If try is green we'll see if Kriyszig is still around to update the diff on Phabricator.

Thanks for the try push Julian. I'm a little occupied this week so I'll rebase the patch over the weekend and update on Phabricator.
Thank you for digging into this.

(In reply to Kriyszig from comment #10)

Thanks for the try push Julian. I'm a little occupied this week so I'll rebase the patch over the weekend and update on Phabricator.
Thank you for digging into this.

Sure thing!

Looks like we still have a lot of failures, but they are now all related to the second issue, ie calls to getCSSDeclarationBlockIssues coming from a sync method. Looking at the try failures, the calls to getCSSDeclarationBlockIssues occur from a variety of stacks (element selection, mutation etc...).

For now I don't have a good idea on how to handle this other than try/catching errors.

Here's a TRY push with a patch to swallow errors: https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=RIy70tEjT4SK8tBIADESMQ.0&revision=1627610490af1ace7832b53a94eedeefd7e1de13

Some known intermittents but a concerning issue on Linux debug on devtools/client/aboutdebugging/test/browser/browser_aboutdebugging_devtoolstoolbox_contextmenu.js . Will have to be investigated before landing.

The bug assignee didn't login in Bugzilla in the last 7 months.
:jdescottes, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: lelouch.cpp → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(jdescottes)
Flags: needinfo?(jdescottes)
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED

I pushed again to see if we have similar issues today
With Julian's patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1659498#c11 , we get a few failures still: https://treeherder.mozilla.org/jobs?repo=try&revision=15cf50cc7a843fb5fc2923a30785fcf782ef07d4

There's not a ton of tests failing an from what I can tell, they all have the same error:

A promise chain failed to handle a rejection: Connection closed, pending request to server0.conn0.windowGlobal6442450976/compatibility40, type getCSSDeclarationBlockIssues failed

There's probably something we can tweak to avoid this, I'll investigate

This was causing test failures on TRY as we had rejections in getCompatibilityIssues
when the page navigates or the toolbox is being destroyed.
To workaround this, we wrap the call in a try/catch block where we swallow the
error if the toolbox is being destroyed or the page navigates.

Depends on: 1832478
Depends on: 1832815
Depends on: 1833617
Depends on: 1834946
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/655787761ee9
[devtools] Enable CSS Compatibility tooltip in Nightly. r=jdescottes,devtools-reviewers.

Backed out for causing compatibility related failures.

Flags: needinfo?(nchevobbe)
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67217b9c90bd
[devtools] Enable CSS Compatibility tooltip in Nightly. r=jdescottes,devtools-reviewers.

fixed the test and pushed again

Flags: needinfo?(nchevobbe)
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: