workaround for #2659, caused by #2743

[jab]

proof of concept, let's discuss

---

This change is

[jab]

Assigning to @dborkan since @bemasc is on vacation. (Have good vacation!)

[trevj]

OK, been scouring the issues. TL;DR: we need the "resize hack" but this hack results in a janky + button even when the + button's animation is disabled (#2745).

Great job tracking all this, @jab!

This approach seems reasonable to me, so long as we keep pushing on https://bugs.chromium.org/p/chromium/issues/detail?id=612836#c19 (@bemasc how about that minimal reproduce case they asked for five months ago?) - but can we keep the logic in TypeScript, in a Polymer script?

[trevj]

BTW, just to say I think I prefer this over adding a tap handler to every element!

[dborkan]

Few small comments, otherwise I'm OK with this

---

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.

---

src/cca/app/scripts/workarounds.js, line 1 [r1] (raw file):

(function () {

Nit: Keeping this as JavaScript is fine for now since hopefully we can get rid of this file soon, but if this keeps growing let's move it to TypeScript

---

src/cca/app/scripts/workarounds.js, line 24 [r1] (raw file):

    // the uproxy root object instantiated in generic_ui/polymer/root.ts.
    console.debug('got uproxy-root-ready');
    var inviteButton = document.querySelector('uproxy-root /deep/ #inviteButton');

It might be good to add a comment to where inviteButton is defined in root.html, mentioned that it is listened to in workarounds.js (just so nobody deletes it or renames it). Generally we assume that HTML element ids are only used in the .html and .ts file for that Polymer element and nowhere else

---

src/cca/app/scripts/workarounds.js, line 25 [r1] (raw file):

    console.debug('got uproxy-root-ready');
    var inviteButton = document.querySelector('uproxy-root /deep/ #inviteButton');
    var inviteUserPanel = document.querySelector('uproxy-root /deep/ #inviteUserPanel');

It looks like inviteUserPanel isn't really used (you print an error if it's not found, but otherwise don't use it), can it be removed?

---

Comments from Reviewable

[jab]

@trevj @dborkan Thank you for reviewing! Just refactored this into a Polymer element as per @trevj's suggestion in 1bef2e3, and added @dborkan's great suggestions there too, PTAL. It took a bit of reshuffling as expected to figure out how to get things in the right place due to the additional complexity of the Polymer build process; I got it working but please let me know if it should be done differently. I was hoping that this refactoring would allow me to drop the uproxy-root-ready event I originally had to add, and could just assume the required dom is ready by the time the querySelector call runs, but it turns out it isn't. After looking at 1bef2e3, is it still worth implementing this as a Polymer element, or does it just complicate things without buying much in return?

[dborkan]

Cool, this works too. 👍

---

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[trevj]

Mmm, OK, yeah I see what you mean about Polymer-isation almost being more trouble than it's worth. I do prefer it being in TypeScript though.

Separately, one question: could you avoid the uproxy-root-ready notification stuff by having the Polymer element "poll" for the element, i.e. a setTimeout loop until the element does exist. What do you think?

[jab]

If the important thing is just having it in typescript, I think I can do that without making it a Polymer element, should I?

I should note that though this is now a .ts file, it contains no type annotations (and for what it's doing, I think that's fine, but let me know if you think it needs some), so currently the only benefit of making it typescript is that tsc will catch syntax errors and the like at compile time, which of course is still valuable.

Regarding replacing the uproxy-root-ready idea with polling, I think using the event is a cleaner solution that could also be used in the future if any other logic ever needs to be scheduled to run once the <uproxy-root> element has loaded and its ready() method run.

[trevj]

Hmm. Yes, looking at it now I think the important things is having it in TypeScript. Sorry :-/

Regarding polling, whatever you think is fine - I just thought it might help limit the scope of this workaround, i.e. nothing outside of cca/ would have to know about it.

👍 to whatever you think on both questions!

[jab]

Cool, thanks @trevj! Reverted to non-polymer-element (but kept as typescript) in acdf838 and then squashed to the single commit ec508e6. Will merge as soon as we're green!