Regression: Print Preview overlay does not appear on Google Slides
Reported by
nutan.ga...@etouch.net,
Sep 11 2017
|
||||||
Issue descriptionChrome Version : 63.0.3212.0 c3a3481c4b1b08775b477144d1d2873a2a3d9900-refs/heads/master@{#500792} OS:OS: Windows (7,8,8.1,10),Linux (14.04 LTS) ,Mac OS X(10.12.3, 10.12.5) What steps will reproduce the problem? 1. Launch chrome, navigate to https://docs.google.com/presentation/u/0/ and Login with correct credentials 2. Click ob Blank presentation or (+) icon to create a new presentation 2. Click on File option, select Print and observe Actual: Print Preview overlay does not appear Expected: Print Preview overlay should appear This is regression issue, broken in ‘M 63’ and below is the CL, will soon update bisect info Good build:63.0.3206.0 Bad build: 63.0.3207.0 Note: On Mac 10.11.6 version after step 2 tab crash is seen
,
Sep 11 2017
,
Sep 11 2017
Adding RB Label as this is a recent Regression. Please remove if not required. Thank You.
,
Sep 12 2017
Looping ekaramad@ as the reviewer of the CL, for more inputs.
,
Sep 13 2017
IIUC, docs pages download a PDF in response to print and after detecting the user agent, in case of Chrome, they will attach an <embed> (to something) and end up creating a MimeHandlerViewContainer. Then, post messaging 'print' to the <embed> will fire up print preview. Now from what I see, |guest_loaded_| is still |false| when the post message happens. Perhaps we do not even create a MimeHandlerViewGuest which means no one will ever ACK the creation for MimeHandlerViewContainer and hence the post message is never sent (sending the post message won't make sense anyway since there is no guest).
,
Sep 13 2017
I can confirm that dropping |element_size_.IsEmpty()| from this conditional will fix the issue: https://cs.chromium.org/chromium/src/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc?rcl=94b4883e25fdfa02af37648977777ba539bde1c2&l=320 It also does not seem to be regressing bug 761705 . fsamuel@: Could you please confirm if there is any potential errors in doing as I suggested here (in this WIP CL: https://chromium-review.googlesource.com/c/chromium/src/+/666198)?
,
Sep 14 2017
I can confirm that the issue is definitely with the way we handle element size here. This is the repro step I found which does not require the slides page:
1- Create an <embed> of type PDF in javascrip, e.g.,
var e = document.createElement('embed');
e.type = 'application/pdf';
e.src = 'path_to_pdf';
2- Set the width and height to 0.
e.style.width = '0px';
e.style.height = '0px';
3- Attach the embed.
document.body.appendChild(e);
4- Post message print:
e.postMessage({type: 'print'}).
On stable channel, this pops up the print screen. On Canary it does not. By setting the size to 0 before attaching the element we will never create a MimeHandlerViewGuest.
,
Sep 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3a9c415ac75740facaf590ab918d9eae5d6b2301 commit 3a9c415ac75740facaf590ab918d9eae5d6b2301 Author: EhsanK <ekaramad@chromium.org> Date: Tue Sep 19 14:03:03 2017 Use base::Optional for both |view_rect_| and |element_size_| This CL allows creation of MimeHandlerViewGuest even when element size is 0x0. This is specifically important for Google slides pages where an <embed> of size 0 is used for print previewing the page. Essentially, we need to makes sure for such <embed>'s the guest is created (after attaching) and embed.postMessage works fine. Bug : 763812 Change-Id: I7ad0cd0898aee2e1b5d6b15dd9bfdcb100a8e16b Reviewed-on: https://chromium-review.googlesource.com/667900 Commit-Queue: Ehsan Karamad <ekaramad@chromium.org> Reviewed-by: Kevin McNee <mcnee@chromium.org> Reviewed-by: Fady Samuel <fsamuel@chromium.org> Cr-Commit-Position: refs/heads/master@{#502847} [modify] https://crrev.com/3a9c415ac75740facaf590ab918d9eae5d6b2301/content/renderer/browser_plugin/browser_plugin.cc [modify] https://crrev.com/3a9c415ac75740facaf590ab918d9eae5d6b2301/content/renderer/browser_plugin/browser_plugin.h [modify] https://crrev.com/3a9c415ac75740facaf590ab918d9eae5d6b2301/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc [modify] https://crrev.com/3a9c415ac75740facaf590ab918d9eae5d6b2301/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.h
,
Sep 20 2017
Retested above issue in latest chrome canary #63.0.3220.0 on Windows (7,8,8.1,10),Linux (14.04 LTS), Mac (10.12.6) OS and issue seems fixed. Kindly review an attached video.
,
Sep 26 2017
Changing the status to 'Fixed' as the Issue is verified in C#9. Please reopen if there is still work to be done here. Thank you!
,
Sep 26 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/98ee1b0984ac7ac9b4f4513a0f54e6d7785d7418 commit 98ee1b0984ac7ac9b4f4513a0f54e6d7785d7418 Author: EhsanK <ekaramad@chromium.org> Date: Tue Sep 26 20:34:34 2017 Add test for a recent regression in PDF Viewer on slides pages Adding a test for https://crrev.com/502847. Bug: 766982 , 763812 Change-Id: I23bd8d27cb2b83ee527f12a9103f82e8eac0b115 Reviewed-on: https://chromium-review.googlesource.com/682597 Commit-Queue: Ehsan Karamad <ekaramad@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Cr-Commit-Position: refs/heads/master@{#504473} [modify] https://crrev.com/98ee1b0984ac7ac9b4f4513a0f54e6d7785d7418/chrome/browser/pdf/pdf_extension_test.cc [add] https://crrev.com/98ee1b0984ac7ac9b4f4513a0f54e6d7785d7418/chrome/test/data/pdf/post_message_zero_sized_embed.html |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by nutan.ga...@etouch.net
, Sep 11 2017Owner: fsam...@chromium.org
Status: Assigned (was: Unconfirmed)