Closed Bug 1465149 Opened 6 years ago Closed 5 years ago

Delete old JsTerm code

Categories

(DevTools :: Console, task, P1)

task

Tracking

(firefox62 wontfix, firefox70 fixed)

RESOLVED FIXED
Firefox 70
Tracking Status
firefox62 --- wontfix
firefox70 --- fixed

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

Details

(Whiteboard: [console-editor-reserve])

User Story

The JsTerm now uses CodeMirror in order to support syntax highlighting. 
But we had to keep the old code around for people who use screenreaders since the current version of CodeMirror isn't accessible at all.
The CodeMirror team is currently working on a version which will handle those accessibility concerns (See https://discuss.codemirror.net/t/code-editor-screen-reader-accessiblity-survey/1790).
Once we update to this version in devtools, we can remove the old JsTerm code (and only run tests with the new version).

Attachments

(1 file)

Remove the pref that was created in Bug 1463409 and make the CodeMirror input the default
Priority: -- → P2
Whiteboard: [boogaloo-mvp]
Product: Firefox → DevTools
Looks like we have Bug 1473805 which is more suited for boogaloo project.
Here, we can repurpose this into deleting the old jsterm code when the new accessible CodeMirror lands.
User Story: (updated)
Summary: Enable CodeMirror-powered JsTerm everywhere → Delete old JsTerm code
Putting this in boogaloo-reserve, but since we don't have any control/information on when the next version will be released, that might be not part of the project.

That being said, it looks like they at least started to gather the needs of people relying on screenreaders (https://discuss.codemirror.net/t/code-editor-screen-reader-accessiblity-survey/1790), which is great.
User Story: (updated)
Whiteboard: [boogaloo-mvp] → [boogaloo-reserve]
Priority: P2 → P3
The work for the new CodeMirror version (officially) started a couple months ago: https://github.com/codemirror/codemirror.next
Whiteboard: [boogaloo-reserve] → [boogaloo-mvp]
Priority: P3 → P2
Depends on: 1500989
Depends on: 1487096
Priority: P2 → P3
Whiteboard: [boogaloo-mvp] → [boogaloo-reserve]

I provided a build to Jamie where the console always uses CodeMirror-powered JsTerm and he answered:

I just gave this a quick spin. Technically, it's a slight regression since the old input allowed multi-line access (not just a single line at a time), which is particularly relevant for braille users. However, this is also true for Edit as HTML, for example. I also don't think this is going to be a major concern for the majority of users. I'd say go ahead, with the footnote that we should still be looking into "proper" CodeMirror accessibility in the long term.
As a side note, that console auto complete is all kinds of ugly in terms of a11y right now (though it isn't any worse with this patch). It used to be a lot better but has been slowly regressing to the point where I really dislike using the console because of it

I filed Bug 1558405 for the single line issue, and Bug 1558404 for the issue with the autocomplete popup.

Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Type: enhancement → task
No longer depends on: 1487096, 1500989
Blocks: 1558804

Make it depend on Bug 1567174 as there's a consequent refactor going on.

Depends on: 1567174
Blocks: 1554899

Since we modified the Editor to be accessible, we got
the green light to remove the old JsTerm code.
This means we can remove the preference for the codeMirror
input, and clean the WebConsole react app a bit.
This also mean we can avoid running a lot of our tests
twice.
Finally, some test helpers had to take argument with
specific shape to work against old jsterm (e.g. checkInputCompletionValue),
that we can now remove to make tests easier to read.

Depends on D39024

Priority: P3 → P1
Whiteboard: [boogaloo-reserve] → [console-editor-reserve]
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8a342aa73f9
Remove old jsterm code. r=jdescottes.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: