Lighthouse 5.2.0 mistakes browser window width for device width in mobile audit

[tmb-github]

Lighthouse version: 5.2.0
Chrome: Version 78.0.3868.0 (Official Build) canary (64-bit)
OS: Windows 8.1

When performing a mobile audit, Lighthouse measures the mobile agent width against the browser window width and throws an error in the PWA audit:

Content is not sized correctly for the viewport
The viewport size is 412px, whereas the window size is 1366px [or current browser window width].

Provide the steps to reproduce

1. Navigate to any website...even https://www.google.com
2. Open Lighthouse and select Mobile device
3. Select Progressive Web App audit
4. Run audit, then check PWA results. "Content is not sized correctly for the viewport" should be an error.
5. It will complain that the viewport size is 412px but the window size is the width of the browser window.
6. Now resize the width of the browser window and repeat steps 4-5. The "window width" in the report will correspond to the browser width, not the device width.

What is the current behavior?

It incorrectly measures the viewport size against the actual browser window width.

What is the expected behavior?

It should compare the viewport size to the simulated device width.

Environment Information

• Affected Channels:

• Lighthouse version: 5.2.0

• Node.js version:

• Operating System: Windows 8.1

• Chrome: Version 78.0.3868.0 (Official Build) canary (64-bit)

Related issues

[connorjclark]

@paulirish this kinda looks like that Chromium test case that was erroring, that we attributed to the content shell test harness ...

[connorjclark]

Couldn't repro on my mac. Will have to try again on my Windows machine later today. Or - can you confirm this is not specific to Windows?

Could you provide the exact command you use to run LH (if using the CLI) - or are you using DevTools Audits panel?

[tmb-github]

I used the DevTools Audits panel and originally discovered it while auditing https://thomasbrodhead.com . I then discovered the same results obtained while auditing https://www.google.com, etc.

[onur-celik]

same problem here https://piyasa.paratic.com

[hugues-koolsol]

Hello,

Same problem with the mobile audit with https://www.koolsol.com/,
but not with the desktop audit.

With the precedent version of lighthouse, it was the opposite ( message on desktop but not with mobile ).

By the way, https://www.koolsol.com fits inside the screen size in any case so I do not see why this error should appear. Perhaps because there is a timeout() on the resize event and lighthouse does this detection before the timeout fires.

Audit runs on Version 78.0.3872.0 (Build officiel) canary (64 bits) windows10.

[patrickhulce]

I'm able to repro this on mac @connorjclark. Perhaps something with our new emulation handoff isn't working properly?

The screen definitely appears to be mobile but for some reason window.outerWidth is still evaluating to the width of the browser window.

[connorjclark]

@patrickhulce yeah me too. For some reason my canary wasn't updating to the latest version. Definitely related to the latest changes in the emulation handoff :(

[connorjclark]

The innerWidth is correctly set to 412px, but the outerWidth always reverts back to w/e the outerWidth is without any emulation.

I thought there might be a race condition b/wn applying emulation and starting the audit, so I added a wait for 10s, but that didn't change anything.

DevTools emulation itself is perfectly fine. Emulating Nexus 5x shows the correct values:

[window.innerWidth, window.outerWidth]
(2) [412, 412]

[connorjclark]

Using window.screen.{width,height} in ViewportDimensions (instead of window.outer{width,height} works.

[connorjclark]

The protocol suggests that outerWidth is not covered: https://chromedevtools.github.io/devtools-protocol/tot/Emulation#method-setDeviceMetricsOverride

[connorjclark]

screen is technically the entire screen (whereas outerWidth is the OS's window), but I've tested using it instead with various sizes of Chrome. During an audit, it is always equal to innerWidth for pages that define a proper viewport.

[connorjclark]

nvm screen doesnt work, I was using v5.2.0 instead of dt-520. only the latter has the deviceScreenEmulationMethod flag.

this DT emulation business needs to be revisited.

[connorjclark]

The DT frontend ends up sending Emulation.setDeviceMetricsOverride ~8 times. each change to the emulation model does one. relevant code

i thought all that craziness might be causing our issue, so I circumvented the frontend abstractions and sent Emulation.setDeviceMetricsOverride directly (exactly what LH would send if it were doing the emulation). Still broken. I suppose we blame the connection handoff at this point.

[connorjclark]

After digging through some IPCs and printf'ing some stack traces, I've learned that as soon as the audit starts a WidgetMsg_DisableDeviceEmulation ipc is sent, as part of the DevToolsUIBindings::Reattach -> content::DevToolsAgentHostImpl::DetachClient -> content::DevToolsSession::Dispose -> content::protocol::EmulationHandler::Disable() series of calls.

[connorjclark]

I believe @JoelEinbinder's multi-client changes will fix this issue (all the agents won't get disposed for multi-clients, but in the current impl. "there can only be one" client and agent state is lost during handoff).

Notes to self: my hacky way of debugging chromium:

diff --git a/content/browser/devtools/protocol/emulation_handler.cc b/content/browser/devtools/protocol/emulation_handler.cc
index 18b6e546804..4279dc71dfa 100644
--- a/content/browser/devtools/protocol/emulation_handler.cc
+++ b/content/browser/devtools/protocol/emulation_handler.cc
@@ -4,6 +4,8 @@
 
 #include "content/browser/devtools/protocol/emulation_handler.h"
 
+#include <base/debug/stack_trace.h>
+
 #include <utility>
 
 #include "base/strings/string_number_conversions.h"
@@ -392,10 +394,13 @@ void EmulationHandler::UpdateDeviceEmulationState() {
   // WidgetMsg and acknowledgment, as well as plump the acknowledgment back to
   // the EmulationHandler somehow. Mojo callbacks should make this much simpler.
   if (device_emulation_enabled_) {
+    LOG(WARNING) << "connor EmulationHandler::UpdateDeviceEmulationState send enable ipc";
     host_->GetRenderWidgetHost()->Send(new WidgetMsg_EnableDeviceEmulation(
         host_->GetRenderWidgetHost()->GetRoutingID(),
         device_emulation_params_));
   } else {
+    LOG(WARNING) << "connor EmulationHandler::UpdateDeviceEmulationState send disable ipc";
+    base::debug::StackTrace().PrintWithPrefix("connor ");
     host_->GetRenderWidgetHost()->Send(new WidgetMsg_DisableDeviceEmulation(
         host_->GetRenderWidgetHost()->GetRoutingID()));
   }
diff --git a/content/renderer/render_widget.cc b/content/renderer/render_widget.cc
index 5a5e484434e..7cf599d9799 100644
--- a/content/renderer/render_widget.cc
+++ b/content/renderer/render_widget.cc
@@ -8,6 +8,8 @@
 #include <limits>
 #include <utility>
 
+#include <base/debug/stack_trace.h>
+
 #include "base/auto_reset.h"
 #include "base/base_switches.h"
 #include "base/bind.h"
@@ -889,6 +891,7 @@ void RenderWidget::OnEnableDeviceEmulation(
 }
 
 void RenderWidget::OnDisableDeviceEmulation() {
+  LOG(WARNING) << "connor RenderWidget::OnDisableDeviceEmulation";
   screen_metrics_emulator_.reset();
 }
 
@@ -1654,6 +1657,8 @@ void RenderWidget::SetScreenRects(const gfx::Rect& widget_screen_rect,
                                   const gfx::Rect& window_screen_rect) {
   widget_screen_rect_ = widget_screen_rect;
   window_screen_rect_ = window_screen_rect;
+  LOG(WARNING) << "connor RenderWidget::WindowRect window_screen_rect_ = | L" << __LINE__ << ". width " << window_screen_rect_.width();
+  // base::debug::StackTrace().PrintWithPrefix("connor ");
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -2039,11 +2044,14 @@ WebRect RenderWidget::WindowRect() {
     // good to do in this case, and it shouldn't happen - since this size is
     // only really needed for windowToScreen, which is only used for Popups.
     rect = pending_window_rect_;
+    LOG(WARNING) << "connor RenderWidget::WindowRect ret1";
   } else {
     rect = window_screen_rect_;
+    // LOG(WARNING) << "connor RenderWidget::WindowRect ret2";
   }
 
   ScreenRectToEmulatedIfNeeded(&rect);
+  LOG(WARNING) << "connor RenderWidget::WindowRect width ret " << rect.width;
   return rect;
 }
 
@@ -2096,6 +2104,7 @@ void RenderWidget::SetPendingWindowRect(const WebRect& rect) {
   // values.
   if (!for_frame()) {
     window_screen_rect_ = rect;
+    LOG(WARNING) << "connor RenderWidget::WindowRect window_screen_rect_ = | L" << __LINE__ << ". width " << window_screen_rect_.width();
     widget_screen_rect_ = rect;
   }
 }
@@ -2261,6 +2270,7 @@ void RenderWidget::SetWindowRectSynchronously(
 
   widget_screen_rect_ = new_window_rect;
   window_screen_rect_ = new_window_rect;
+  LOG(WARNING) << "connor RenderWidget::WindowRect window_screen_rect_ = | L" << __LINE__ << ". width " << window_screen_rect_.width();
   if (show_callback_) {
     // Tests may call here directly to control the window rect. If
     // Show() did not happen yet, the rect is stored to be passed to the
@@ -2665,6 +2675,7 @@ void RenderWidget::DidAutoResize(const gfx::Size& new_size) {
                         size_.height());
       widget_screen_rect_ = new_pos;
       window_screen_rect_ = new_pos;
+      LOG(WARNING) << "connor RenderWidget::WindowRect window_screen_rect_ = | L" << __LINE__ << ". width " << window_screen_rect_.width();
     }
 
     // TODO(ccameron): Note that this destroys any information differentiating
diff --git a/content/renderer/render_widget_screen_metrics_emulator.cc b/content/renderer/render_widget_screen_metrics_emulator.cc
index 29d798db005..00f09c7a271 100644
--- a/content/renderer/render_widget_screen_metrics_emulator.cc
+++ b/content/renderer/render_widget_screen_metrics_emulator.cc
@@ -40,6 +40,8 @@ void RenderWidgetScreenMetricsEmulator::Apply() {
   VisualProperties modified_visual_properties = original_visual_properties_;
   applied_widget_rect_.set_size(gfx::Size(emulation_params_.view_size));
 
+  LOG(WARNING) << "connor " << applied_widget_rect_.width();
+
   if (!applied_widget_rect_.width())
     applied_widget_rect_.set_width(original_size().width());
 
diff --git a/third_party/blink/renderer/devtools/front_end/audits/AuditsPanel.js b/third_party/blink/renderer/devtools/front_end/audits/AuditsPanel.js
index a156a070d13..51a98bb0a9f 100644
--- a/third_party/blink/renderer/devtools/front_end/audits/AuditsPanel.js
+++ b/third_party/blink/renderer/devtools/front_end/audits/AuditsPanel.js
@@ -248,7 +248,7 @@ Audits.AuditsPanel = class extends UI.Panel {
    * We set the device emulation on the DevTools-side for two reasons:
    * 1. To workaround some odd device metrics emulation bugs like occuluding viewports
    * 2. To get the attractive device outline
-   * flags.emulatedFormFactor is always set to none, so Lighthouse doesn't apply its own emulation.
+   *
    * We also set flags.deviceScreenEmulationMethod = 'provided' to let LH only apply UA emulation
    */
   async _setupEmulationAndProtocolConnection() {
diff --git a/third_party/blink/renderer/devtools/front_end/audits_worker/lighthouse/lighthouse-dt-bundle.js b/third_party/blink/renderer/devtools/front_end/audits_worker/lighthouse/lighthouse-dt-bundle.js
index 954aadb1bfb..d764ddabf2b 100644
--- a/third_party/blink/renderer/devtools/front_end/audits_worker/lighthouse/lighthouse-dt-bundle.js
+++ b/third_party/blink/renderer/devtools/front_end/audits_worker/lighthouse/lighthouse-dt-bundle.js
@@ -22431,6 +22431,7 @@ if(!this._shouldToggleDomain(domainCommand[1],enable)){
 return Promise.resolve();
 }
 }
+console.log(method,...params);
 return this._connection.sendCommand(method,...params);
 }
 
@@ -24127,6 +24128,11 @@ const artifacts={};
 try{
 await driver.connect();
 
+const res=await driver.evaluateAsync('JSON.stringify([window.innerWidth, window.outerWidth])');
+console.log('!!!!',res);
+debugger;
+
+
 
 await GatherRunner.loadBlank(driver);
 
diff --git a/third_party/blink/renderer/devtools/front_end/emulation/DeviceModeModel.js b/third_party/blink/renderer/devtools/front_end/emulation/DeviceModeModel.js
index 27510aac0ed..d01573012f0 100644
--- a/third_party/blink/renderer/devtools/front_end/emulation/DeviceModeModel.js
+++ b/third_party/blink/renderer/devtools/front_end/emulation/DeviceModeModel.js
@@ -607,6 +607,7 @@ Emulation.DeviceModeModel = class extends Common.Object {
       mobile,
       screenOrientation,
       resetPageScaleFactor) {
+    // debugger;
     screenSize.width = Math.max(1, Math.floor(screenSize.width));
     screenSize.height = Math.max(1, Math.floor(screenSize.height));
 
diff --git a/third_party/blink/web_tests/http/tests/devtools/audits/audits-successful-run.js b/third_party/blink/web_tests/http/tests/devtools/audits/audits-successful-run.js
index 165f1354d20..16c06e27783 100644
--- a/third_party/blink/web_tests/http/tests/devtools/audits/audits-successful-run.js
+++ b/third_party/blink/web_tests/http/tests/devtools/audits/audits-successful-run.js
@@ -32,8 +32,6 @@
     'uses-responsive-images',
     'uses-text-compression',
     'uses-webp-images',
-    // content shell issues
-    'content-width' // crbug.com/987722
   ];
 
   TestRunner.addResult('Tests that audits panel works.\n');

my_chrome --enable-logging=stderr --v=1 2>&1 | grep connor

[connorjclark]

CL: https://chromium-review.googlesource.com/c/chromium/src/+/1743411/

[connorjclark]

The multi-client CL just landed in Chromium. This issue will be resolved in the next canary Chrome. The first stable Chrome to resolve this issue will be Chrome 78, ETA Oct 22.

[Joelsz]

In DevTools on the latest Canary (78.0.3894.0) - where this issue is resolved so we can once again test mobile - the Service Worker audit started failing when auditing mobile on www.bhphotovideo.com: "Does not register a service worker that controls page and start_url".

@connorjclark Can it be related to this?

[connorjclark]

@Joelsz, I see the same in Stable Chrome, so it's not related to this change.

but, I did think this change could have been at fault - I am not as confident making big changes in DevTools (as opposed to for the CLI channel) because we don't have tons of test coverage in DevTools. But I am working on that - #9501 is the first step.

As a sanity check, I confirmed that ToT chrome still passes all the audts on this site (except the apple thing, @exterkamp rookie numbers man ...) https://100.exterkamp.codes/

[Joelsz]

In stable chrome you're still having the issue where it audits desktop in the place of mobile, though it shouldn't make a difference for the Service Worker since it's shared by domain.

Interestingly, I get the SW audit result random when using stable chrome and with "No throttling" it always passes. I'm thinking that it might be related to what delay the service registration might have so lighthouse can't find it early enough.

Anyway thanks for your response @connorjclark.

[connorjclark]

Ah, of course, I admit I forgot about that. We have a few different things getting fixed in DevTools recently...

I did try with the CLI v5.2.0 (this doesn't use anything related to the DT changes made here), and still see that start_url fails.

Interestingly, I get the SW audit result random when using stable chrome and with "No throttling" it always passes. I'm thinking that it might be related to what delay the service registration might have so lighthouse can't find it early enough.

yeah seems like a too-late detection issue.
this might be a related discussion: #9551

[patrickhulce]

FYI @Joelsz www.bhphotovideo.com start_url specifically should be covered by #9450 and fixed by #9451 to be released in the next version of Lighthouse.

[Joelsz]

Yes, I noticed the start_url issue is being resolved in @patrickhulce PR, just wanted to point out the audit under "Installable". Will definitely check out the linked related discussion. Thanks.