Consider removing marker-offset

RESOLVED FIXED in Firefox 52

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
MDN says we don't implement it (https://developer.mozilla.org/en-US/docs/Web/CSS/marker-offset), but we have style code for it. (`getComputedStyle(document.documentElement).markerOffset` returns auto, and there's code for it in layout/style).

There doesn't seem to be any layout code for this. We should remove it.
Comment hidden (mozreview-request)
(Assignee)

Comment 2

a year ago
I removed it from the WPT test as well since it's an unspecced property (used to be part of a draft or something, no longer exists in the spec).
Comment on attachment 8803292 [details]
Bug 1311959 - Remove marker-offset;

https://reviewboard.mozilla.org/r/87604/#review86550

r+ for the layout and testing part. The devtools part need someone from devtools team to review.

IIRC, some parts of them are maintained out of tree, so you should not touch them here.
Attachment #8803292 - Flags: review?(xidorn+moz) → review+
Attachment #8803292 - Flags: review?(ttromey)
:tromey, please review the devtools part. I suppose most of that part is actually maintained out of tree. And in that case, please suggest where should we file issue for removing them.

Comment 5

a year ago
mozreview-review
Comment on attachment 8803292 [details]
Bug 1311959 - Remove marker-offset;

https://reviewboard.mozilla.org/r/87604/#review86652

Thank you.  Devtools is in a funny state where different parts are maintained
according to different rules right now.  So, see below for what to change.

::: devtools/client/debugger/new/bundle.js
(Diff revision 1)
>  	  "margin-top": {
>  	    inherited: false,
>  	    supports: 3,
>  	    values: ["-moz-calc", "auto", "calc", "inherit", "initial", "unset", ],
>  	  },
> -	  "marker-offset": {

Please just drop the change to the debugger bundle and instead just file a bug here:

https://github.com/devtools-html/debugger.html/issues

I don't think this code is actually used by the debugger anyhow.

::: devtools/client/sourceeditor/codemirror/codemirror.bundle.js:11801
(Diff revision 1)
>  	    "inline-box-align", "justify-content", "left", "letter-spacing",
>  	    "line-break", "line-height", "line-stacking", "line-stacking-ruby",
>  	    "line-stacking-shift", "line-stacking-strategy", "list-style",
>  	    "list-style-image", "list-style-position", "list-style-type", "margin",
>  	    "margin-bottom", "margin-left", "margin-right", "margin-top",
> -	    "marker-offset", "marks", "marquee-direction", "marquee-loop",
> +	    "marks", "marquee-direction", "marquee-loop",

This is fine but please also file a bug here:

https://github.com/codemirror/CodeMirror/issues
Attachment #8803292 - Flags: review?(ttromey) → review+
(Assignee)

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8803292 [details]
Bug 1311959 - Remove marker-offset;

https://reviewboard.mozilla.org/r/87604/#review86652

> Please just drop the change to the debugger bundle and instead just file a bug here:
> 
> https://github.com/devtools-html/debugger.html/issues
> 
> I don't think this code is actually used by the debugger anyhow.

https://github.com/devtools-html/debugger.html/issues/975

> This is fine but please also file a bug here:
> 
> https://github.com/codemirror/CodeMirror/issues

https://github.com/codemirror/CodeMirror/issues/4340
Comment hidden (mozreview-request)
For the record, the spec is:
https://www.w3.org/TR/2008/REC-CSS2-20080411/generate.html#markers

It was dropped in CSS 2.1 (relative to CSS 2.0) due to lack of implementation.

Comment 9

a year ago
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a621c81ab1a2
Remove marker-offset; r=tromey,xidorn

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a621c81ab1a2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.