Bug 188607 – EWS bubbles are being hidden due to lack of space.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ross Kirsling]

EWS bubbles are being hidden due to lack of space.

[Ross Kirsling]

Created attachment 347175 [details]
Patch

[Ross Kirsling]

Created attachment 347177 [details]
Screenshot

Here's a screenshot showing a typical case followed by a particularly bad case (with the proposed height increase).

We could alternatively keep bumping the width up, but I figure an extra row will give us room to grow.

[Ryosuke Niwa]

Comment on attachment 347175 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347175&action=review

> Websites/bugs.webkit.org/ChangeLog:10
> +        * template/en/default/attachment/list.html.tmpl:
> +        Double the height so that we have an extra row to work with.

Can we just extend the width instead? I don't want every patch to occupy more vertical space.

[Ross Kirsling]

Created attachment 347195 [details]
Patch

[Ross Kirsling]

(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 347175 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347175&action=review
> 
> > Websites/bugs.webkit.org/ChangeLog:10
> > +        * template/en/default/attachment/list.html.tmpl:
> > +        Double the height so that we have an extra row to work with.
> 
> Can we just extend the width instead? I don't want every patch to occupy
> more vertical space.

Sure -- it seems that we need at least 710px in order to cover the "bad case" in my screenshot, though I'm sure it could get even worse (say, if the queues are backed up at a given moment and we have lots of "#10"-type numbers showing), so perhaps 750px? It doesn't look great in a tiny window, but it does get the job done for now...

[Ryosuke Niwa]

Another approach is for EWS bubble page to postMessage the width of bubbles to the parent window (in JS) and then Bugzilla page to adjust the bubble iframe width accordingly.

[Fujii Hironori]

Bug 187853 is for height:20px.

[Ross Kirsling]

Created attachment 347216 [details]
Patch

[Ross Kirsling]

(In reply to Ryosuke Niwa from comment #6)
> Another approach is for EWS bubble page to postMessage the width of bubbles
> to the parent window (in JS) and then Bugzilla page to adjust the bubble
> iframe width accordingly.

I've attempted this by mimicking the code for the Review Patch screen, but I'm not sure how to test this locally.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #9)
> (In reply to Ryosuke Niwa from comment #6)
> > Another approach is for EWS bubble page to postMessage the width of bubbles
> > to the parent window (in JS) and then Bugzilla page to adjust the bubble
> > iframe width accordingly.
> 
> I've attempted this by mimicking the code for the Review Patch screen, but
> I'm not sure how to test this locally.

Er, hmm. This is definitely not going to work, since there's no allowance for multiple EWS iframes on one page (since that's not something the Review Patch screen needs to worry about). I'm going to obsolete this patch and unobsolete the previous one for now.

[Fujii Hironori]

How about this solution?
https://stackoverflow.com/a/32162488/2691131

[Ross Kirsling]

(In reply to Fujii Hironori from comment #11)
> How about this solution?
> https://stackoverflow.com/a/32162488/2691131

That's the same-origin solution, but unfortunately, the iframe domain is webkit-queues.webkit.org. :(

If you open the console on this page and execute...
> document.getElementsByClassName('statusBubble')[0].getElementsByTagName('iframe')[0].contentWindow.document

...you'll see this error:
> SecurityError: Blocked a frame with origin "https://bugs.webkit.org" from accessing a cross-origin frame. Protocols, domains, and ports must match.

[Fujii Hironori]

Can you do the following in both documents to relax the SOP?

> document.domain = "webkit.org";

https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy#Changing_origin

[Ross Kirsling]

(In reply to Fujii Hironori from comment #13)
> Can you do the following in both documents to relax the SOP?
> 
> > document.domain = "webkit.org";
> 
> https://developer.mozilla.org/en-US/docs/Web/Security/Same-
> origin_policy#Changing_origin

Interesting, I didn't know one could "promote" their origin from subdomain to domain.
I think that would work, by adding a line here:
https://github.com/WebKit/webkit/blob/master/Tools/QueueStatusServer/templates/statusbubble.html#L54

But I'm not sure whether Apple folks would be okay with such an approach?

[Ross Kirsling]

Created attachment 347325 [details]
Patch

[Ross Kirsling]

Comment on attachment 347325 [details]
Patch

Alright, another attempt at using the containerMetrics postMessage -- this time, I'm allowing an element ID to be passed back and forth, so that a page with multiple EWS iframes can keep track of each one.

I think this approach should be valid, but I'm not certain whether it's possible for me to test, so I think I will need some support on that part. 🙇‍♂️

[Alexey Proskuryakov]

Yes, we can do testing on a separate instance once the change is complete.

[Fujii Hironori]

Comment on attachment 347325 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347325&action=review

> Websites/bugs.webkit.org/template/en/default/attachment/edit.html.tmpl:42
> +    $(`.statusBubble`)[0].style.height = e.data.height;

Don't you need "+px"?
e.data.height + "px";

[Fujii Hironori]

JFYI, you can use evt.source to distinguish the source iframe.
> iframe.contentWindow === e.source

I uploaded the example.
https://output.jsbin.com/fulovogoci

[Ross Kirsling]

(In reply to Fujii Hironori from comment #18)
> Comment on attachment 347325 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347325&action=review
> 
> > Websites/bugs.webkit.org/template/en/default/attachment/edit.html.tmpl:42
> > +    $(`.statusBubble`)[0].style.height = e.data.height;
> 
> Don't you need "+px"?
> e.data.height + "px";

Hmm, I was just mimicking this:
https://github.com/WebKit/webkit/blob/master/Websites/bugs.webkit.org/code-review.js#L543-L544

But yes, (as your example shows) it seems like 'px' should be necessary...

> JFYI, you can use evt.source to distinguish the source iframe.
> > iframe.contentWindow === e.source
> 
> I uploaded the example.
> https://output.jsbin.com/fulovogoci

Awesome! That approach will certainly be simpler. :D

[Ross Kirsling]

Created attachment 347557 [details]
Patch

[Ross Kirsling]

Created attachment 347559 [details]
Patch

[EWS Watchlist]

Comment on attachment 347559 [details]
Patch

Attachment 347559 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8927713

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html

[EWS Watchlist]

Created attachment 347623 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ross Kirsling]

Comment on attachment 347559 [details]
Patch

I believe this patch should be good to go now.

[Daniel Bates]

Comment on attachment 347559 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347559&action=review

> Websites/bugs.webkit.org/template/en/default/attachment/edit.html.tmpl:50
> +<script>
> +window.addEventListener('message', function (e) {
> +    if (e.origin !== 'https://webkit-queues.webkit.org' || !e.data.height)
> +        return;
> +
> +    const iframe = document.querySelector('.statusBubble > iframe');
> +    iframe.style.height = e.data.height + 'px';
> +    iframe.style.width = e.data.width + 'px';
> +}, false);
> +
> +function handleStatusBubbleLoad(iframe) {
> +    iframe.contentWindow.postMessage('containerMetrics', 'https://webkit-queues.webkit.org');
> +}
> +</script>

This code effectively duplicates similar code in code-review.js. I suggest that we extract the common code into a shared JavaScript function and put it in its own JavaScript file that we include from this page and code-review.js. Maybe something like:

function updateFromStatusBubbleFrame(messageEvent)
 {
    if (messageEvent.origin !== 'https://webkit-queues.webkit.org')
        return;
    if (!messageEvent.data || !messageEvent.data.height)
        return;

    var mayBeFrames = Array.prototype.slice.call(document.getElementsByClassName('statusBubble'));
    if (!mayBeFrames)
        return;

    const iframeTagName = 'IFRAME';
    function filterFrames(element, index, array) {
        if (element.tagName === iframeTagName)
            return true;
        if (element.firstElementChild && element.firstElementChild.tagName === iframeTagName)
            return true;
        return false;
    }

    var frameToUpdate = null;
    for (const frame of mayBeFrames.filter(filterFrames)) {
        if (frame.contentWindow === messageEvent.source) {
            frameToUpdate = frame;
            break;
        }
    }
    if (!frameToUpdate)
        return;

    frameToUpdate.style.height = e.data.height + 'px';
    frameToUpdate.style.width = e.data.width + 'px';
 }

Then in both this file and code-review.js we would register this function as the event handler for the DOM MessageEvent:

window.addEventListener('message', updateFromStatusBubbleFrame, false);

> Websites/bugs.webkit.org/template/en/default/attachment/edit.html.tmpl:277
>          <div class="statusBubble">
>            <iframe src="https://webkit-queues.webkit.org/status-bubble/[% attachment.id %]"

If we can move the 'class="statusBubble"' from the <div> to the <iframe> then we can simplify the code in updateFromStatusBubbleFrame() above.

> Websites/bugs.webkit.org/template/en/default/attachment/edit.html.tmpl:278
> +                  style="width: 600px; height: 20px; border: none;" scrolling="no" onload="handleStatusBubbleLoad(this);">

This is OK as is. The semicolon (;) is not necessary.

> Websites/bugs.webkit.org/template/en/default/attachment/list.html.tmpl:53
> +window.addEventListener('message', function (e) {
> +    if (e.origin !== 'https://webkit-queues.webkit.org' || !e.data.height)
> +        return;
> +
> +    for (const iframe of document.querySelectorAll('.statusBubble > iframe')) {
> +        if (iframe.contentWindow !== e.source)
> +            continue;
> +
> +        iframe.style.height = e.data.height + 'px';
> +        iframe.style.width = e.data.width + 'px';
> +    }
> +}, false);

This code is similar to code in code-review.js. See my suggestion above for how we can share this code.

[Ross Kirsling]

Created attachment 347964 [details]
Patch

[Ross Kirsling]

Created attachment 347967 [details]
Patch

[Daniel Bates]

Comment on attachment 347967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347967&action=review

> Websites/bugs.webkit.org/js/status-bubble.js:35
> +        iframe.style.width = event.data.width + 'px';

Once we set the style for the frame we can stop the iteration. The version I wrote in comment 26 did this.

[Ross Kirsling]

(In reply to Daniel Bates from comment #29)
> Comment on attachment 347967 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347967&action=review
> 
> > Websites/bugs.webkit.org/js/status-bubble.js:35
> > +        iframe.style.width = event.data.width + 'px';
> 
> Once we set the style for the frame we can stop the iteration. The version I
> wrote in comment 26 did this.

Good point! Will add to patch for landing.

[Ross Kirsling]

Created attachment 347969 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 347969 [details]
Patch for landing

Clearing flags on attachment: 347969

Committed r235262: <https://trac.webkit.org/changeset/235262>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43664670>

[Ryosuke Niwa]

Hm... it looks like we now don't set the border to be 0 in patch review page so bubbles show a border. We should probably also set the width to be 0, 0 initially to avoid FOC.

[Ross Kirsling]

(In reply to Ryosuke Niwa from comment #35)
> Hm... it looks like we now don't set the border to be 0 in patch review page
> so bubbles show a border. We should probably also set the width to be 0, 0
> initially to avoid FOC.

The cause of this seems to be that we're requesting code-review.js with a 'version' query parameter...

The old file is this:
https://bugs.webkit.org/code-review.js?version=48

The new file is accessible either by dropping the query parameter or by incrementing it:
https://bugs.webkit.org/code-review.js
https://bugs.webkit.org/code-review.js?version=49

...though I honestly can't tell whether "version 49" is a real thing or just an out-of-bounds value.

Shall I just drop the query parameter?

[Ross Kirsling]

(In reply to Ross Kirsling from comment #36)
> (In reply to Ryosuke Niwa from comment #35)
> > Hm... it looks like we now don't set the border to be 0 in patch review page
> > so bubbles show a border. We should probably also set the width to be 0, 0
> > initially to avoid FOC.
> 
> The cause of this seems to be that we're requesting code-review.js with a
> 'version' query parameter...
> 
> The old file is this:
> https://bugs.webkit.org/code-review.js?version=48
> 
> The new file is accessible either by dropping the query parameter or by
> incrementing it:
> https://bugs.webkit.org/code-review.js
> https://bugs.webkit.org/code-review.js?version=49
> 
> ...though I honestly can't tell whether "version 49" is a real thing or just
> an out-of-bounds value.
> 
> Shall I just drop the query parameter?

Scratch that, it's simply cached. Viewing the Review Patch screen in a private window, everything already seems to be working as desired.

[Michael Catanzaro]

*** Bug 187853 has been marked as a duplicate of this bug. ***