Migrate printpreviewtoolbar binding to a customized built-in Custom Element

RESOLVED FIXED in Firefox 63

Status

()

P3
normal
RESOLVED FIXED
10 months ago
3 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)

(Reporter)

Description

10 months ago
This one is just a normal toolbar with some extra logic around it.
Priority: -- → P3
Comment hidden (mozreview-request)
(Reporter)

Comment 2

7 months ago
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.
(Assignee)

Comment 3

7 months ago
(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
(Reporter)

Comment 4

7 months ago
(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]
(Reporter)

Comment 5

7 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 7

7 months ago
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.
(Assignee)

Comment 9

7 months ago
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.
(Assignee)

Comment 11

7 months ago
(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.
(Assignee)

Comment 12

7 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 14

7 months ago
(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)
(Assignee)

Comment 17

7 months ago
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)

Updated

7 months ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
(Assignee)

Comment 18

7 months ago
Created attachment 8987899 [details]
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.
(Assignee)

Updated

7 months ago
Summary: Investigate removing printpreviewtoolbar binding → Migrate printpreviewtoolbar binding to a customized built-in Custom Element

Comment 19

7 months ago
mozreview-review
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+

Comment 20

7 months ago
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 hidden (mozreview-request)

Comment 22

7 months ago
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
Comment hidden (mozreview-request)
(Assignee)

Comment 25

7 months ago
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)

Comment 26

7 months ago
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

Comment 27

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71d21ed3f88e
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Assignee)

Updated

7 months ago
Blocks: 1473130
Depends on: 1473265
(Assignee)

Updated

3 months ago
See Also: → bug 1498740
You need to log in before you can comment on or make changes to this bug.