Migrate printpreviewtoolbar binding to a customized built-in Custom Element

RESOLVED FIXED in Firefox 63

Status

()

task
P3
normal
RESOLVED FIXED
Last year
2 months ago

People

(Reporter: ntim, Assigned: bgrins)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [ntim-backlog])

Attachments

(2 attachments)

This one is just a normal toolbar with some extra logic around it.
Priority: -- → P3
Since the toolbar is only used once in the UI, it might be better to inline the markup directly onto the toolbar, and attach the listeners using JS, rather than having a custom element built for this.
(In reply to Tim Nguyen :ntim from comment #2)
> Since the toolbar is only used once in the UI, it might be better to inline
> the markup directly onto the toolbar, and attach the listeners using JS,
> rather than having a custom element built for this.

That's probably true. I didn't spend much time on this so I'm not sure how much more work it would be to do it that way.

If you're interested in taking this over the finish line let me know - otherwise I'll probably continue as a CE since this already looks pretty green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15c0601f83799ba1c9a7447dc20bfdedc99e1d0b
(In reply to Brian Grinstead [:bgrins] from comment #3)
> If you're interested in taking this over the finish line let me know -
> otherwise I'll probably continue as a CE since this already looks pretty
> green:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=15c0601f83799ba1c9a7447dc20bfdedc99e1d0b

Putting this on my list.
Whiteboard: [ntim-backlog]
Hm, I didn't realize the print preview was loaded in browser.xul and not in the print window, so inlining is probably not possible.

Although, since it's used once, it's might be possible to use a simple JS class with the toolbar node as constructor argument, without the custom elements bit. There wouldn't be a lot a benefit over the current approach though. It would allow some tabbrowser.js style cleanups, but I doubt they will be worth it for print preview.
I'm wondering why this binding is in toolkit/components since the only consumer anywhere seems to be in browser.xul. I'd suggest that we move the folder over to browser/components as a follow up if we are generally happy with the approach here.
Bob, I see you have some changes in the file history of printUtils.js and printPreviewBindings.xml. Would you be a good person to review the overall approach here?

This is changing things so that instead of creating a `<toolbar printpreview=true>` and attaching the print preview XBL binding to that element, we now create `<toolbar is="printpreview-toolbar">` and attach a Custom Element to that node. The Custom Element is loaded inside PrintUtils.init(), and the migration from XML->JS is mostly automated. All the tests pass and after doing some light manual testing stuff looks/feels alright, but I'm not sure if there are other things we should be testing, or any edge cases to be aware of.
Flags: needinfo?(bobowencode)
If bobowen doesn't feel comfortable with it, or doesn't want to do it, or you want to short-circuit, I can look at it.
(In reply to Richard Marti (:Paenglab) from comment #8)
> TB uses it too:
> https://searchfox.org/comm-central/search?q=printPreviewBindings.
> xml&case=false&regexp=false&path=

Ah OK, interesting that the binding isn't attached somewhere in toolkit/. Seems reasonable to keep the styles in toolkit then - either as a separate CSS file in the toolkit/components folder (and hook up an empty CSS file for OSX) or directly into win/linux global.css instead of browser.css as the patch here does.
If we wanted this to work without manually doing subscript loading in the consumer it looks like we could use setElementCreationCallback - that seems to support the customized-built-in case as per https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/tests/mochitest/webcomponents/test_custom_element_set_element_creation_callback.html#89.

I didn't realize that would work originally - I'll probably change the patch to use that instead.
(In reply to Brian Grinstead [:bgrins] from comment #12)
> If we wanted this to work without manually doing subscript loading in the
> consumer it looks like we could use setElementCreationCallback - that seems
> to support the customized-built-in case as per
> https://searchfox.org/mozilla-central/rev/
> 39b790b29543a4718d876d8ca3fd179d82fc24f7/dom/tests/mochitest/webcomponents/
> test_custom_element_set_element_creation_callback.html#89.
> 
> I didn't realize that would work originally - I'll probably change the patch
> to use that instead.

It seems to work perfectly - pushed up a new version of the patch that shows that approach. I'm not sure which approach is best in this case, since this is in components/ and not content/ like most of the other bindings (and the customElements.js script).
Comment on attachment 8987269 [details]
Bug 1450813 - Create print preview toolbar as customized built-in Custom Element;

https://reviewboard.mozilla.org/r/252502/#review259414

Glad that you've found a bug that make the current in-tree features useful.

I don't think we should reference the element definition from `browser.js`: this is a feature-specific binding, it should be loaded from the script of the feature. But perhaps this kind of centralization and componentization is not in the scope of this bug (and it was referenced from `browser.css` from the first place), so I am fine with landing this as-is.
Attachment #8987269 - Flags: review?(timdream) → review+
(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #10)
> If bobowen doesn't feel comfortable with it, or doesn't want to do it, or
> you want to short-circuit, I can look at it.

Thanks Mike, I was just about to suggest you. :-)
Looks like Tim has picked it up anyway.
Flags: needinfo?(bobowencode)
Comment on attachment 8987269 [details]
Bug 1450813 - Create print preview toolbar as customized built-in Custom Element;

Mike: Tim did a review for the Custom Element migration itself, but I'd still like a review of the overall approach (see Comment 9)
Attachment #8987269 - Flags: review?(mconley)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comparison on Win7 of the UI with (left) and without (right) the patch. Did a manual test of individual controls and didn't notice any difference.
Summary: Investigate removing printpreviewtoolbar binding → Migrate printpreviewtoolbar binding to a customized built-in Custom Element
Comment on attachment 8987269 [details]
Bug 1450813 - Create print preview toolbar as customized built-in Custom Element;

https://reviewboard.mozilla.org/r/252502/#review260462

Approach seems okay to me! From my experience with this thing, if something was to have gone wrong, it would have been pretty obvious when attempting to switch into print preview. So if everything was working alright, and if our tests still pass on Linux and Windows, I think we're alright.
Attachment #8987269 - Flags: review?(mconley) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s a88bc1cfb711551d61c7a00052fa7376499804dd -d 25344c934be3: rebasing 470872:a88bc1cfb711 "Bug 1450813 - Create print preview toolbar as customized built-in Custom Element;r=mconley,timdream" (tip)
merging browser/base/content/browser.css
merging toolkit/themes/linux/global/global.css
merging toolkit/themes/linux/global/jar.mn
merging toolkit/themes/windows/global/global.css
warning: conflicts while merging browser/base/content/browser.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee4e2bfffe18
Create print preview toolbar as customized built-in Custom Element;r=mconley,timdream
A querySelector in the test needed to be updated. Did a TV try push to confirm: https://treeherder.mozilla.org/#/jobs?repo=try&revision=451b4aaacc42f44c0fe22b71f0758134d10fd99a.
Flags: needinfo?(bgrinstead)
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71d21ed3f88e
Create print preview toolbar as customized built-in Custom Element;r=mconley,timdream
https://hg.mozilla.org/mozilla-central/rev/71d21ed3f88e
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1473265
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.