1450813 - Migrate printpreviewtoolbar binding to a customized built-in Custom Element

Status: RESOLVED FIXED

(Toolkit :: Printing, task, P3)

Description

[Tim Nguyen :ntim]

This one is just a normal toolbar with some extra logic around it.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1450813 - Create print preview toolbar as customized built-in Custom Element;

Review commit: https://reviewboard.mozilla.org/r/252502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252502/

Comment 2

[Tim Nguyen :ntim]

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.

Comment 3

[Brian Grinstead [:bgrins]]

(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

Comment 4

[Tim Nguyen :ntim]

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

Comment 5

[Tim Nguyen :ntim]

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.

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8987269 [details]
Bug 1450813 - Create print preview toolbar as customized built-in Custom Element;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252502/diff/1-2/

Comment 7

[Brian Grinstead [:bgrins]]

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.

Comment 8

[Richard Marti (:Paenglab)]

TB uses it too: https://searchfox.org/comm-central/search?q=printPreviewBindings.xml&case=false&regexp=false&path=

Comment 9

[Brian Grinstead [:bgrins]]

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.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 11

[Brian Grinstead [:bgrins]]

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

Comment 12

[Brian Grinstead [:bgrins]]

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.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8987269 [details]
Bug 1450813 - Create print preview toolbar as customized built-in Custom Element;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252502/diff/2-3/

Comment 14

[Brian Grinstead [:bgrins]]

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

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 16

[Bob Owen (:bobowen)]

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

Comment 17

[Brian Grinstead [:bgrins]]

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)

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: comparison-with-and-without-change.png

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.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 20

[Mozilla Autoland]

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)

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8987269 [details]
Bug 1450813 - Create print preview toolbar as customized built-in Custom Element;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252502/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee4e2bfffe18
Create print preview toolbar as customized built-in Custom Element;r=mconley,timdream

Comment 23

[Andrei Ciure[:aciure]]

Backed out 1 changesets (bug 1450813) for failing browser_page_change_print_original.js

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ee4e2bfffe18b6e8136dc8fa10bd4625344f5661&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=186016760&filter-searchStr=Windows+7+pgo+Mochitests+with+e10s+test-windows7-32-pgo%2Fopt-mochitest-browser-chrome-e10s-3+M-e10s%28bc3%29

log: https://treeherder.mozilla.org/logviewer.html#?job_id=186016760&repo=autoland

backout: https://hg.mozilla.org/integration/autoland/rev/e242648cae8c3b1b89d44183a31230a9690584a5

Comment 24

[Brian Grinstead [:bgrins]]

Comment on attachment 8987269 [details]
Bug 1450813 - Create print preview toolbar as customized built-in Custom Element;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252502/diff/4-5/

Comment 25

[Brian Grinstead [:bgrins]]

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.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71d21ed3f88e
Create print preview toolbar as customized built-in Custom Element;r=mconley,timdream

Comment 27

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/71d21ed3f88e