Consider removing <isindex> from the parser and form submission [tor 18914]

RESOLVED FIXED in Firefox 56

Status

()

Core
HTML: Parser
P2
normal
RESOLVED FIXED
a year ago
15 days ago

People

(Reporter: Domenic Denicola, Assigned: hsivonen)

Tracking

(Blocks: 4 bugs, {dev-doc-complete, site-compat})

Trunk
mozilla56
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: btpp-active [tor][fingerprinting])

MozReview Requests

()

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160407164938

Steps to reproduce:

Per https://github.com/whatwg/html/issues/1088 we are discussing removing <isindex> support from the HTML Standard, given Blink and EdgeHTML's successful removals already. Would Gecko be interested in following along?
(Reporter)

Updated

a year ago
OS: Unspecified → All
Hardware: Unspecified → All

Updated

a year ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Let's get Henri's opinion here.
Flags: needinfo?(hsivonen)
Whiteboard: btpp-fixlater

Updated

a year ago
See Also: → bug 616027

Updated

a year ago
Repeating what I said in https://github.com/w3c/html/issues/240
No issue from a Web Compatibility point of view.
(Assignee)

Comment 3

a year ago
Let's remove it.

It's a bit sad to give up support for historical HTML, but this feature is pretty annoying implementation-wise.

Also, the removal means there's one fewer locale leaks for Tor Browser to deal with.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Flags: needinfo?(hsivonen)
Whiteboard: btpp-fixlater → btpp-active
(Assignee)

Comment 4

a year ago
(FWIW, stuff like https://groups.google.com/a/chromium.org/d/msg/blink-dev/14q_I06gwg8/52oBtr2VCAAJ was the reason why I wasn't eager to be on the front lines of removing this.)

Domenic, to save me the trouble of searching Blink sources:
What exactly does "removing" mean? Making the parser treat <isindex> as completely unknown or still treating it as a void element like <bgsound>?

Comment 5

a year ago
Completely unknown per https://codereview.chromium.org/96653004/patch/90001/100050. Note that per https://codereview.chromium.org/96653004/patch/90001/100047 and https://codereview.chromium.org/96653004/patch/90001/100052 they also removed the corresponding support for <input name=isindex> as part of https://codereview.chromium.org/96653004/. We should probably do the same.

Comment 6

a year ago
Based on Henri's confirmation we've gone ahead and removed it from HTML: https://github.com/whatwg/html/commit/5c44abc734eb483f9a7ec79da5844d2fe63d9c3b. Three out of four browsers ought to do it.
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/isindex-support-will-be-removed/
Keywords: dev-doc-needed, site-compat
(Assignee)

Comment 8

a year ago
Created attachment 8748742 [details] [diff] [review]
WIP (lacks test case changes)

Comment 9

a year ago
Tests added to web-platform-tests in

https://github.com/w3c/web-platform-tests/pull/3684

and html5lib-tests in

https://github.com/html5lib/html5lib-tests/pull/74
Whiteboard: btpp-active → btpp-active [tor][tor 18914]
Blocks: 1260929
Dropped in the latest Safari Tech Preview: https://developer.apple.com/safari/technology-preview/release-notes/
(Assignee)

Comment 11

9 months ago
ehsan, I didn't get around to adding telemetry the last time round this was discussed. Given comment 10, are you OK with proceeding without telemetry? We're now the last major engine to remove this.
(Assignee)

Comment 12

9 months ago
And actually neeinfoing ehsan now. ehsan, please see the previous comment.
Flags: needinfo?(ehsan)
No, please add a telemetry probe first.

The last time we removed a Gecko only feature (remote jar: URLs, bug 1215235) we ended up breaking IBM iNotes and had to do a quick dot release (bug 1255139.)  It really isn't worth it having to go through another one of those firedrills and lose more users over it.
Flags: needinfo?(ehsan)

Updated

7 months ago
Blocks: 1330892

Updated

7 months ago
Priority: -- → P2
Summary: Consider removing <isindex> from the parser and form submission → Consider removing <isindex> from the parser and form submission [tor 18914]
Whiteboard: btpp-active [tor][tor 18914] → btpp-active [tor][fingerprinting]
(Assignee)

Comment 14

4 months ago
And now not having removed this already is getting in the way of fixing bug 1355479. :-(
(Assignee)

Updated

4 months ago
Depends on: 1356181

Updated

4 months ago
Blocks: 1356381, 1121467
(Assignee)

Comment 15

2 months ago
The telemetry dashboard for release 54 says 36.15 million sessions without isindex submission versus 8 (just 8, no multiplier) sessions with at least one isindex submission.

I think it's safe to conclude that we can remove <isindex>.
(Assignee)

Updated

2 months ago
Blocks: 1347643
(Assignee)

Updated

2 months ago
Attachment #8748742 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8883503 - Flags: review?(wchen)
(Assignee)

Comment 19

2 months ago
Created attachment 8884288 [details] [diff] [review]
htmlparser patch

Attaching the htmlparser patch, too, to make it easier to land this (if needed) while I'm away from Bugzilla.

Comment 20

a month ago
mozreview-review
Comment on attachment 8883503 [details]
Bug 1266495 - Remove <isindex>.

https://reviewboard.mozilla.org/r/154420/#review161024
Attachment #8883503 - Flags: review?(wchen) → review+

Comment 21

a month ago
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f999b725ef69
Remove <isindex>. r=wchen
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/isindex-support-has-been-dropped/
(Assignee)

Comment 23

a month ago
https://hg.mozilla.org/projects/htmlparser/rev/40a176e227d284d4ce8b66dd5fe4577860f5f563
Mozilla bug 1266495 - Remove <isindex> from the C++ side, too. r=wchen.

Comment 24

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f999b725ef69
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

a month ago
Depends on: 1382269
I've clearly marked it as obsolete on its reference page:

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/isindex

And added a note to the Fx56 rel notes:

https://developer.mozilla.org/en-US/Firefox/Releases/56#Removals_from_the_web_platform

Let me know if these sound OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.