expose ::marker pseudo-element in inspector

RESOLVED FIXED in Firefox 68

Status

defect
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: aja, Assigned: Kwan)

Tracking

68 Branch
Firefox 68
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(12 attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
20.79 KB, image/png
Details
Reporter

Description

3 months ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

select a <li> or a <summary> element in inspector

Actual results:

no ::marker pseudo-element is exposed under the element

Expected results:

::marker pseudo-element should be exposed under the element

Status: UNCONFIRMED → NEW
Component: Untriaged → Inspector
Ever confirmed: true
Product: Firefox → DevTools
Summary: expose ::marker pseudo-class in inspector → expose ::marker pseudo-element in inspector
Depends on: 205202
Assignee

Comment 1

3 months ago

Note: this is currently exposable by toggling devtools.inspector.showAllAnonymousContent to true in about:config, but it displays with the raw <_moz_generated_content_marker></_moz_generated_content_marker> appearance. Fixing it to display as ::marker would be pretty easy given the existing code for doing the same to ::before and ::after, but do we actually want to show it all the time for all <li> & <summary> elements? Or would that be too noisy?

Also at the moment dragging & dropping a node before ::markers leads to crashes:
bp-2fc640fe-43fa-4454-a555-ef02e0190327
That needs handling here:
https://searchfox.org/mozilla-central/rev/a7315d78417179b151fef6108f2bce14786ba64d/devtools/client/inspector/markup/markup.js#2161-2197

Assignee

Comment 2

3 months ago

At the moment when devtools.inspector.showAllAnonymousContent is true shadow
hosts with ::marker and ::before pseudos will stop showing the ::before and
won't show the ::marker either.

Assignee

Comment 6

3 months ago

Prior to this the tab would crash with signature [@ nsINode::InsertChildBefore ]
if trying to insert before the ::marker.

Assignee

Comment 7

3 months ago

Where there is existing for ::before and ::after.

Assignee

Comment 9

3 months ago

So ::marker pseudo elements can be found via searching for "::marker" in the
markup view.

Assignee

Comment 10

3 months ago

For autocompletion of : in selector contexts, like the Style Editor.

Assignee

Comment 11

3 months ago

So the current stack gets it into a good state where after toggling the anon pref:

  • ::befores on shadow hosts don't disappear if the host as a ::marker as well
  • rules are shown in the rule view when a ::marker is selected
  • the tab doesn't crash when drag & dropping a node before the ::marker
  • ::markers are displayed in the markup view with the proper pseudo representation instead of the internal generated content tag name (and in the breadcrumbs)
  • ::markers can be found via searching "::marker" in the markup view

I still need to add more tests, especially one for the drag & drop crash.
Then we need to decide if we should always show markers, or only after a pref-flip (presumably a new pref, separate to the anon one, perhaps even with UI)

Assignee: nobody → moz-ian
Status: NEW → ASSIGNED
Assignee

Comment 12

3 months ago

Though I don't currently understand how those functions are used or how to test
them.

Assignee

Comment 13

3 months ago

Instead of only when setting devtools.inspector.showAllAnonymousContent.

Assignee

Comment 14

3 months ago

Try builds available for experimentation here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a21727618d5244ed2d93f834b63afad7dd839d

These are to the top of the patch stack, so ::markers display by default. Please test it out and report any feedback, any noticed bugs, and whether or not always having ::markers display creates too much noise or seems fine.

Reporter

Comment 15

3 months ago

(In reply to Ian Moody [:Kwan] (UTC+0) from comment #14)

Try builds available for experimentation here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a21727618d5244ed2d93f834b63afad7dd839d

These are to the top of the patch stack, so ::markers display by default. Please test it out and report any feedback, any noticed bugs, and whether or not always having ::markers display creates too much noise or seems fine.

Thanks, looks good to me after testing the win64 Try build. Will update here if I spot anything odd.
Not too noisy as far as the reporter is concerned.

Assignee

Comment 16

3 months ago

So one immediate disadvantage is that list items whose contents are just a short text node will no longer be shown inline. Attachment shows the difference on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/li

Reporter

Comment 17

3 months ago

I suppose you could, in the case of short text node, display the ellipsis button before(in LTR)/after(in RTL) the short text run.
Seems to me like a lot of extra work for little gain, but I guess it'd be doable.

Severity: normal → enhancement
Assignee

Updated

3 months ago
See Also: → 1540066
Assignee

Updated

3 months ago
See Also: → 1540071
Attachment #9054084 - Attachment description: Bug 1539265 - Handle ::marker pseudos in getSelectorFromGrip and translateNodeFrontToGrip. → Bug 1539265 - Handle ::marker pseudo-elements in reps.
Assignee

Comment 18

3 months ago

A surprisingly-green try here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74b34fa91e8893a11f0c8cc069a2c4839b862335

(I'm sure something will go wrong on landing to make up for it)

Keywords: checkin-needed

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmptNRphF\npatching file devtools/client/shared/components/reps/reps.js\nHunk #1 FAILED at 1115\nHunk #2 FAILED at 1238\nHunk #3 FAILED at 1353\nHunk #4 FAILED at 1485\nHunk #5 FAILED at 1532\nHunk #6 FAILED at 1575\nHunk #7 FAILED at 1625\nHunk #8 succeeded at 2453 with fuzz 2 (offset 361 lines).\nHunk #18 FAILED at 2856\nHunk #20 FAILED at 2961\nHunk #30 FAILED at 3731\nHunk #31 FAILED at 3769\nHunk #32 FAILED at 3781\nHunk #33 FAILED at 4064\nHunk #34 FAILED at 4074\nHunk #35 FAILED at 4185\nHunk #36 FAILED at 4217\nHunk #37 FAILED at 4431\nHunk #38 FAILED at 4889\nHunk #40 FAILED at 5078\nHunk #41 FAILED at 5143\nHunk #42 FAILED at 5165\nHunk #43 FAILED at 5180\nHunk #44 FAILED at 5470\nHunk #45 FAILED at 5481\nHunk #46 FAILED at 6348\nHunk #47 FAILED at 6517\nHunk #48 FAILED at 6529\nHunk #49 FAILED at 7093\nHunk #50 FAILED at 7314\nHunk #51 FAILED at 7375\nHunk #52 FAILED at 7566\n31 out of 52 hunks FAILED -- saving rejects to file devtools/client/shared/components/reps/reps.js.rej\nabort: patch failed to apply', '')

Flags: needinfo?(moz-ian)
Assignee

Comment 20

3 months ago

Clash with bug 1540239 which also touched the auto-generated devtools/client/shared/components/reps/reps.js. Auto rebase was never going to handle that. Rebased and re-generated devtools/client/shared/components/reps/reps.js.

Flags: needinfo?(moz-ian)
Keywords: checkin-needed

Comment 21

3 months ago

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/931e98129878
Handle ::marker pseudo-elements on shadow roots in walker.js. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/72bbb4b28932
Add a test for ::marker displaying in the markup view. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/637a9d6af0f3
Handle ::marker pseudo-elements in css-logic.js, so their rules show in the rule view. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/19c8e404ac5f
Add isMarkerPseudoElement function to layout/utils.js. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/8015aeb5279d
Handle ::marker pseudos during drag & drop of nodes in the markup view. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/3add7d4d4853
Add a few optimisations for ::marker pseudos. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/6eaf5bdd945c
Don't represent ::marker pseudo-elements with the internal _moz_generated_content_marker tagName in the markup view. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/18fa7e971a0d
Add ::marker pseudo-element handling to WalkerIndex. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/f78146b8aa17
Add ::marker to the list of suggested pseudo selectors. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/be9f5f73bdfe
Handle ::marker pseudo-elements in reps. r=jdescottes,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/8206b880f9ec
Make ::marker pseudo-elements always visible in the markup view. r=jdescottes,mbalfanz,pbro

Keywords: checkin-needed
Assignee

Comment 23

3 months ago

Ah, missed that test since I was only doing try fuzzy 'linux64 'devtools. Fortunately found the inbuilt try --preset debugger-tests now.

Green try (with one bug 1507337 orange, and shippable-related artifact bustages):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdbd270e5a2baf9ec8e15885c481b5d52b9d9769&selectedJob=237466073

Flags: needinfo?(moz-ian)
Keywords: checkin-needed

Comment 24

3 months ago

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e17935240c9
Handle ::marker pseudo-elements on shadow roots in walker.js. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/ed3ec6d0ecb2
Add a test for ::marker displaying in the markup view. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/15349b80d230
Handle ::marker pseudo-elements in css-logic.js, so their rules show in the rule view. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/f65cad0863f9
Add isMarkerPseudoElement function to layout/utils.js. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/25c6f5583502
Handle ::marker pseudos during drag & drop of nodes in the markup view. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/c4a552cb33e3
Add a few optimisations for ::marker pseudos. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/ad3465ab22b6
Don't represent ::marker pseudo-elements with the internal _moz_generated_content_marker tagName in the markup view. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/0b3e62a77748
Add ::marker pseudo-element handling to WalkerIndex. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/98af58faaa11
Add ::marker to the list of suggested pseudo selectors. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/32d6fbf8449a
Handle ::marker pseudo-elements in reps. r=jdescottes,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/fb4a79e4b5f9
Make ::marker pseudo-elements always visible in the markup view. r=jdescottes,mbalfanz,pbro

Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.