Closed Bug 1596812 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Installer, enhancement, P1)

Desktop
Windows
enhancement

Tracking

()

RESOLVED FIXED
Firefox 77
Install Update Workflow In Review
Tracking Status
firefox77 --- fixed

People

(Reporter: mhowell, Assigned: agashlin)

References

(Depends on 1 open bug)

Details

Attachments

(10 files)

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
198.85 KB, image/png
Details
21.96 KB, image/png
Details
541.09 KB, image/png
Details

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.

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.

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.

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

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).

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

Install Update Workflow: --- → In Review
Summary: Web-based stub installer UI → Web-based stub installer UI (HTML/JS/CSS)
Blocks: 1617957

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.

Flags: needinfo?(mhowell)

(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.

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.

Flags: needinfo?(mhowell)

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.

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.

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.

Assignee: mhowell → agashlin

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

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.

Flags: needinfo?(jteh)

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.

Flags: needinfo?(agashlin)

(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.

Flags: needinfo?(agashlin)

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:

Thanks!

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.

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.

(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.

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").

Flags: needinfo?(jteh)

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.

Flags: needinfo?(jteh)

(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.

Flags: needinfo?(jteh)

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:

Official branding:

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.

Flags: a11y-review+
Pushed by mhowell@mozilla.com:
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

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.

Flags: needinfo?(mhowell)

(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.

Flags: needinfo?(mhowell)

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.

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.

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

See Also: → 1631547
See Also: → 1631781
Regressions: 1632315
Regressions: 1632462
Regressions: 1632463
Regressions: 1632981
You need to log in before you can comment on or make changes to this bug.