1596812 - Web-based stub installer UI (HTML/JS/CSS)

Status: RESOLVED FIXED

(Firefox :: Installer, enhancement, P1)

Description

[Molly Howell (she/her) (mostly inactive)]

Currently the stub installer's user interface code is highly fragile, error-prone, difficult to modify, and difficult to read. The number of engineers available to work on it is worryingly small, and it's extremely difficult to bring up new engineers because the code is so specialized. Replacing the current implementation with web pages rendered by a native web view will make modifying the UI and/or the visual design much faster and easier, so that specialist engineers would no longer be needed to make any changes, and changes can be made more frequently and with less risk. It would also remove a lot of limits that currently exist on what UI elements may be used and how they can be customized.

Note that currently "web-based" means "using HTML and CSS," not actually downloading any UI elements from the web at runtime.

Comment 1

[Axel Hecht [:Pike]]

That sounds very interesting from an l10n point of view. The installer is on the list of things stuck on very old hacks l10n-wise, and where we'd like to Fluent all-the-things. We'd love to be involved early in the design here to discuss the options.

Comment 2

[Molly Howell (she/her) (mostly inactive)]

Well, the problem is that the entire UI can't actually be converted; we still need some native message boxes and the web control isn't able to set the window title, so there still need to be some strings directly accessible to NSIS. So for now I'm planning on sticking with the current system, but this will make it possible to migrate most of the strings at a later time.

Comment 3

[Molly Howell (she/her) (mostly inactive)]

Here's a document I wrote that explains more about the rationale for this and why I've gone with the solution I have.

Comment 4

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1596812 Part 1 - Update our custom nsisui.exe. r=agashlin

Minify this file by removing the dialogs we don't need and hide all the
unnecessary controls in the one we do need, so the stub installer code
doesn't have to do that manually (I would have removed those controls
altogether, but the NSIS compiler errors out if you do that).

Comment 5

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1596812 Part 2 - NSIS WebBrowser plugin. r=agashlin

This is all the code and build files for an NSIS plugin that enables
rendering a web page as the content of an NSIS dialog.

Documentation and the compiled binary are in later commits in this series.

Depends on D56576

Comment 6

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1596812 Part 3 - Compiled binary for the WebBrowser plugin. r=agashlin

Depends on D56577

Comment 7

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1596812 Part 4 - Add the WebBrowser plugin to the installer build files. r=agashlin

Depends on D56579

Comment 8

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1596812 Part 5 - Add the web content files and include them in the installer build. r=agashlin

Depends on D56580

Comment 9

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1596812 Part 6 - Replace the stub installer UI code with calls to the web plugin. r=agashlin

Depends on D56581

Comment 10

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1596812 Part 7 - Add documentation for the stub installer web UI. r=agashlin

Depends on D56582

Comment 11

[James Teh [:Jamie]]

Is this at a stage where we can try it out, and if so, how? I'd like to do some early a11y review if I can, as this is something we really need to get right for a11y. Thanks.

Comment 12

[Molly Howell (she/her) (mostly inactive)]

(In reply to James Teh [:Jamie] from comment #11)

Is this at a stage where we can try it out, and if so, how? I'd like to do some early a11y review if I can, as this is something we really need to get right for a11y. Thanks.

I don't have any builds yet (the reviews aren't quite finished), but I agree that's important, so I'll try to get you something as soon as I can. Leaving needinfo open until then.

Comment 13

[Molly Howell (she/her) (mostly inactive)]

Okay, here's a try build you can use (taken from this push). Might be a few last late-breaking changes (and of course I'll address anything you find), but that's very close to final. Since it's a one-off build, you'll probably get Windows SmartScreen complaining about; just ignore it.

Comment 14

[James Teh [:Jamie]]

I took this for a spin. There are definitely some problems which need to be addressed here.

1. When the installer starts, focus goes to an unlabelled Win32 Button, which might actually be invisible.
2. Tabbing moves focus to an HWND with class "Shell DocObject View". The HTML document (class: "Internet Explorer_Server") is a child HWND of the focus. That means a screen reader can't read the document because the document HWND doesn't get focus.
3. If you tab again, you get to another unlabelled Win32 Button. Tabbing yet again takes you to an unlabelled Win32 check box (Button with BS_CHECKBOX). These are probably not visible either. The next press of tab cycles around to Shell DocObject View.

The actual web document is fine, including the progress bar. 👍

What needs to happen:

1. Focus should start in the Internet Explorer_Server HWND.
2. I'm guessing the buttons and check box aren't meant to be user perceivable. Either:
   • They need to be removed. But I'm guessing there's some NSIS limitation here?
   • They should be removed from the tab order by removing WS_TABSTOP. Again, not sure if that's possible with NSIS.
   • Given we're focusing the document as per 1), perhaps we can intercept keydown events in the web document with JS to prevent tab from moving focus to these controls? That's risky, though, because it could mess with things if we ever have focusable controls in the web content. We'd have to be very careful to fake the tab order in that case.

Comment 15

[Molly Howell (she/her) (mostly inactive)]

That's extremely good to know, thank you. I knew something was wrong with the focus, but hadn't identified exactly what. I'll experiment and see what I can do with those superfluous controls; you're right that the framework doesn't easily allow just removing them, but we should still have options.

Comment 16

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

The checkbox shouldn't be visible, that can be changed in nsisui.exe. It looks like NSIS itself shows the back button and focuses another (which is not even visible) when moving pages, so that will have to be overridden as we were doing before.

I was able to get tab focus working within the profile refresh page by calling IOleInPlaceActiveObject::TranslateAccelerator() for the active object in the message loop. The focus still cycles out of the web page, though, even if there is no other control to go to. And it isn't clear how/if the browser object is supposed to become deactivated, currently it stays active no matter where focus goes.

Here's a stub with the changes I've made so far, from this try push. Not sure what to look into next.

Comment 17

[Axel Hecht [:Pike]]

I glanced at this, and AFAICT we're removing accesskeys on some UI controls. Is that intentional?

Comment 18

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Jamie, could you please try out these two builds?

• I'm particularly interested in build 1 (from this try job), which forces the profile refresh page to show up, though it doesn't do the refresh. This page is the only really interactive part, I've mostly fixed using Tab to cycle between the inputs, but focus also visits the whole browser control at the end.
• Build 2 (from this try job) will run normally, usually going straight on to begin the download and install.

---

(In reply to Axel Hecht [:Pike] from comment #17)

I glanced at this, and AFAICT we're removing accesskeys on some UI controls. Is that intentional?

Yes, this is intended. The accesskey property on the HTML form elements does something similar, but it would require some additional work to extract the key from the label and correctly show an underline for that letter. As we now have Tab focus cycling working, this should be less of an issue, but if it is considered essential I could look into it again.

Comment 19

[Rachel Tublitz [:rachel]]

Attachment: side by side with existing installer

Adam, it looks like there's also a few (very) small UI differences:

1. The checkbox in this version is not aligned with the bottom of the "Restore default settings..." text. (It's super close, like 1px off)

2. The "built for people, not for profit" text is actually a bit smaller in this version. Look at where the "...profit" sentence ends, and you can see it.

I'd also like to better understand the accesskeys decision here. Can you outline what's being removed specifically, so we can decide if it's worth getting a fix in? I'm concerned about introducing regressions here, specifically ones that might make the installer harder to use from an a11y perspective.

Comment 20

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(In reply to Rachel Tublitz [:rachel] from comment #19)

Adam, it looks like there's also a few (very) small UI differences:

1. The checkbox in this version is not aligned with the bottom of the "Restore default settings..." text. (It's super close, like 1px off)

2. The "built for people, not for profit" text is actually a bit smaller in this version. Look at where the "...profit" sentence ends, and you can see it.

I'll look into tuning these.

I'd also like to better understand the accesskeys decision here. Can you outline what's being removed specifically, so we can decide if it's worth getting a fix in? I'm concerned about introducing regressions here, specifically ones that might make the installer harder to use from an a11y perspective.

The convention on Windows is for every control to have an access key that you use to focus it with Alt + accesskey, and holding down Alt shows that key as an underlined letter in the text label. It's very easy to do this with Windows native controls, just write the label with the ampersand before the letter (Mailing &address) and it is taken care of. In HTML there is an accesskey attribute that can be used for the same input, and in IE that uses the same key combination. However, specifying and rendering the key ourselves will be a bit involved:

• IME: I don't know how this works when the letter isn't represented by a single key.
• rendering: I don't know how to ensure the right part of the string is being underlined, unicode code points are probably insufficient. We can maybe detect holding Alt in order to conditionally render the hints but I'm not sure.
• localization: If we're specifying this in the same way (with &), that won't change how l10n proceeds, but if we can't find a way to programmatically decide the key and rendering, it may be necessary to introduce a hack such as specifying the access key and the parts of the string before and after it as separate strings.

And all of this is in order to choose between a button and a checkbox on a page of the stub installer which most users won't use.

The other general keyboard navigation method, cycling focus with Tab, should be completely usable in this case (and will be perfect with a workaround I'll have up in a few minutes). But I am not an accessibility expert, so I don't know if it's acceptable to skimp here, and there may be relatively easy ways to do this in HTML that I don't know about.

Comment 21

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

That extra Tab glitch was driving me crazy, I've implemented the least-hacky workaround I could think of and updated the patch on Phabricator.

:Jamie, please disregard the comment 18 builds. Could you please try these instead:

• Build 1 (from try) - forcing profile refresh page (but not action)
• Build 2 (from try) - normal

Thanks!

Comment 22

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: fixed checkbox alignment

I added vertical-align: middle to the checkbox in the aurora, nightly, and unofficial builds (as they're all identical CSS). Official has its own layout where the alignment looks ok already. Full range of builds coming soon.

Comment 23

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: official refresh page comparison

I should note that in all cases the checkbox appears in line with the first line of text in, rather than centrally aligned with the whole block of text as was previously the case. I think this makes sense, this was a limitation of the Windows checkbox form control rather than an intentional decision, but here's a screenshot showing the difference for the record.

Comment 24

[Molly Howell (she/her) (mostly inactive)]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #23)

I should note that in all cases the checkbox appears in line with the first line of text in, rather than centrally aligned with the whole block of text as was previously the case. I think this makes sense, this was a limitation of the Windows checkbox form control rather than an intentional decision, but here's a screenshot showing the difference for the record.

The other major difference visible in this comparison is the color of the button; as with the checkbox alignment, the original design called for the button to be blue, and it would have been blue in the native implementation if it had been reasonably possible to do, so the reason for changing it here is to bring back the original design intent.

Comment 25

[James Teh [:Jamie]]

Sorry for the delay.

Build 1: The profile refresh page is great. The button gets focus as expected and the tab order is nice. However, as soon as you press that button, the invisible Win32 "Button" gets focus, and you can't even tab back into the document (class: "Internet Explorer_Server").

Build 2: Same problem: the invisible Win32 "Button" gets focus, and you can't even tab into the document (class: "Internet Explorer_Server").

Comment 26

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Thanks for being patient with me, it's good to know that the interactive controls are working. I've tried some comparisons between the old stub installer and this new one, using Windows's built-in Narrator and the Microsoft "Inspector" tool to follow the focus, and I think the new behavior is roughly the same as it was before. Before it would also focus on the button, now you can tab to the useless "pane", but in both cases nothing useful is in focus.

Not to say that this awkwardness is acceptable, but I'm not sure what should happen when there is nothing interactive on the page to focus on, IE doesn't seem to let you focus on the page otherwise.

I can try synthesizing a focus. In the following build, I added tabindex="0" to the "Now installing..." label and the progress bar, and focus starts on the label. I also removed the need to focus the cancel button. The refresh page is always shown but the download never begins, for inspection of the installing page. Stub here (from try)

If this isn't an improvement, I don't know what to try next.

Comment 27

[James Teh [:Jamie]]

(In reply to Adam Gashlin (he/him) [:agashlin] from comment #26)

Not to say that this awkwardness is acceptable, but I'm not sure what should happen when there is nothing interactive on the page to focus on, IE doesn't seem to let you focus on the page otherwise.

That's odd. In a full IE browser, the document is certainly focusable, and I've definitely seen embedded MSHTML where this works. That said, I've also seen similar problems to this in some other embedded cases, so there's clearly some weirdness concerning focus on the document.

I can try synthesizing a focus. In the following build, I added tabindex="0" to the "Now installing..." label and the progress bar, and focus starts on the label.

This is much better. It's still a bit weird that we have to focus the label (especially because it gets exposed with a role of grouping), but it's a significant improvement and I think we can live with it.

If this isn't an improvement, I don't know what to try next.

The only other thing I could suggest is calling SetFocus on the Internet Explorer_Server window directly (which is a child of the Shell DocObject View window). I know you're dealing with NSIS, but it can call user 32 functions, so this might be possible.

Comment 28

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Thanks! I had some trouble trying to get that window at the appropriate time, though it's probably something we could get through some object's IOleWindow::GetWindow(). Though even if it did get focus, Tab would lose it again. If you can think of an MSHTML embedding where you've seen this work I could check it out, there's probably something deep in MFC that would work here. But for now I'm going to leave it as-is.

There were a few RTL issues that came up, those should now be fixed.

Here are the builds for testing:

Nightly branding:

• English (en-US) (try task)
• Russian (ru, longer) (try task)
• Hebrew (he, RTL) (try task)

Official branding:

• en-US (try task)
• ru (try task)
• he (try task)

This should be landing soon if nothing major comes up, but as some of the strings changed slightly (removing the access key &) it may be a little while before they are fully localized. I modified those strings for the above repacks. Hebrew is currently missing STUB_INSTALLING_BODY so that's still in English.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32eba50a6ae1
Part 1 - Update our custom nsisui.exe. r=agashlin
https://hg.mozilla.org/integration/autoland/rev/1c80f6c6d43b
Part 2 - NSIS WebBrowser plugin. r=agashlin,nalexander
https://hg.mozilla.org/integration/autoland/rev/860657b3ae14
Part 3 - Compiled binary for the WebBrowser plugin. r=agashlin
https://hg.mozilla.org/integration/autoland/rev/c6debaebcb1f
Part 4 - Add the WebBrowser plugin to the installer build files. r=agashlin,nalexander
https://hg.mozilla.org/integration/autoland/rev/7dd034d966f2
Part 5 - Add the web content files and include them in the installer build. r=agashlin,nalexander,mconley
https://hg.mozilla.org/integration/autoland/rev/c75138f9f267
Part 6 - Replace the stub installer UI code with calls to the web plugin. r=agashlin
https://hg.mozilla.org/integration/autoland/rev/0cc9ef01b1cf
Part 7 - Add documentation for the stub installer web UI. r=agashlin,nalexander

Comment 30

[Zibi Braniecki [:zbraniecki][:gandalf]]

Congratulations on landing!

Coming back to Comment 2 - what's the plan for moving away from .properties for the Web installer and replacing them with Fluent DOM annotations? I'm trying to scale down .properties use in anticipation of deprecation and this looks like a quite big new feature that uses .properties.

Comment 31

[Molly Howell (she/her) (mostly inactive)]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #30)

Congratulations on landing!

Thanks!

Coming back to Comment 2 - what's the plan for moving away from .properties for the Web installer and replacing them with Fluent DOM annotations? I'm trying to scale down .properties use in anticipation of deprecation and this looks like a quite big new feature that uses .properties.

We do not currently have a plan, unfortunately. There's one big challenge there and also one big opportunity.
The challenge is that even though the main UI is now web-based, we still have some localized strings that do not go through that implementation, specifically those used in the window title and a couple of native message boxes, so those can't be migrated directly. That prevents us from just dropping the entire current localization system and doing everything in Fluent DOM instead.
The opportunity is the fact we're not actually using the .properties file directly during runtime; there's a Python script that runs during the installer build and converts that file into a format called NLF that the NSIS UI framework uses as its native format, and that's what the run time code gets all its strings from. That at least gives us a nice single point where we can swap out the thing we're converting from in favor of something else entirely, like an .ftl file; obviously that would be some work, but not as much as doing a Fluent implementation as an NSIS plugin. So I think our best way forward might be to update that script.

Comment 32

[Zibi Braniecki [:zbraniecki][:gandalf]]

That prevents us from just dropping the entire current localization system and doing everything in Fluent DOM instead.

It might we worth considering switching everything else to Fluent, so that we can establish and test that Fluent pathway works and iron out any bugs or limitations we encounter.
That would leave just a small subset of strings going through the old system.

So I think our best way forward might be to update that script.

https://github.com/projectfluent/python-fluent should be able to help here.

Comment 33

[Axel Hecht [:Pike]]

Looking at the target, we might benefit from transpiling Fluent to old-school js. https://github.com/eemeli/fluent-compiler is the inspiration here, though it's not reviewed, and creates modern js. We might need something different.

I don't think that NLF files are an easy option, as they do a different and incompatible variable placement.

Comment 34

[Molly Howell (she/her) (mostly inactive)]

I'm gonna file a separate bug where we can talk about this more.

Comment 35

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/32eba50a6ae1
https://hg.mozilla.org/mozilla-central/rev/1c80f6c6d43b
https://hg.mozilla.org/mozilla-central/rev/860657b3ae14
https://hg.mozilla.org/mozilla-central/rev/c6debaebcb1f
https://hg.mozilla.org/mozilla-central/rev/7dd034d966f2
https://hg.mozilla.org/mozilla-central/rev/c75138f9f267
https://hg.mozilla.org/mozilla-central/rev/0cc9ef01b1cf