1397250 - "Load diff" in github PR fails to load (with dom.webcomponents.enabled flipped to true)

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect, P3)

Description

[kdavis]

Attachment: Page before clicking on "Load diff"

Steps to repo.

1. Launch Firefox Nightly 57.0a1 (2017-09-05) (64-bit)
2. Navigate to https://github.com/mozilla/DeepSpeech/pull/805/files
3. Scroll down the page to any "Load diff" link
4. Click on link

Expected Behaviour:
The diff should be rendered.

Observed Behaviour: 
The ""Load diff" turns in to a greyed out "Loading..." text which is never replaced by the requested diff.


Note:
Doing the same process on Chrome yields the expected behaviour.

Comment 1

[kdavis]

Attachment: Page after clicking on "Load diff"

Comment 2

[:Gijs (he/him)]

WFM. Can you re-test on a clean profile? Any errors in the browser console (cmd-shift-j) when this happens?

Comment 3

[Kohei Yoshino [:kohei]]

WFM.

Comment 4

[:Gijs (he/him)]

(In reply to Kohei Yoshino [:kohei] from comment #3)
> WFM.

Err, we should probably keep this open until we have confirmation it also works for the reporter - and/or we figure out what is causing it not to work for them while it works for others.

Comment 5

[kdavis]

Attachment: WebConsole Headers

1. On a clean profile is works for me on nightly
2. With my current profile the only thing I see in the browser console is a GET that has no response (See Attachement) on nightly

Comment 6

[kdavis]

Attachment: Web Console Response

Comment 7

[Carmen Fat [:cfat] - Ecosystem QA]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0

Hi,

I also tried to reproduce the issue using latest Nightly 57.0a1 (2017-09-10) on macOS 10.12 but didn't manage to reproduce it.  

Kdavis, can you please attach here your "about:support" page data? You can do this by navigating to "about:support", click on "Copy raw data to clipboard" button and paste it in a ".txt". Maybe the information from "about:support" can help us in our further investigations.

Thank you.

Comment 8

[kdavis]

Attachment: about:support raw data

Response to need info with about:support raw data attached as a txt file

Comment 9

[Carmen Fat [:cfat] - Ecosystem QA]

I looked over the provided information from the "about:support" attached but didn't see anything unusual.
:Gijs can you please take a look at the "about:support" data and see if anything stands out there?

Thank you!

Comment 10

[:Gijs (he/him)]

(In reply to Carmen Fat [:carmenf] from comment #9)
> I looked over the provided information from the "about:support" attached but
> didn't see anything unusual.
> :Gijs can you please take a look at the "about:support" data and see if
> anything stands out there?
> 
> Thank you!

I don't see anything in there that would shed light on this off-hand. :kdavis, can you check the *browser* console when this happens (cmd-shift-j) - not the web console? Does it reproduce in safe mode (Help > Restart with add-ons disabled) on the profile where you're seeing this? Thanks for helping us track this down!

Comment 11

[:Gijs (he/him)]

Ah, I take that back, you've got web components enabled and I can reproduce if I toggle that.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

dom.webcomponents.enabled was added for deprecated experimental implementation. We plan to turn dom.webcomponents.enabled off in bug 1398981 and remove the preference in bug 1400762. Given that, this is very low priority for us, not plan to fix it but happy to take a patch.

Comment 13

[:Gijs (he/him)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> dom.webcomponents.enabled was added for deprecated experimental
> implementation. We plan to turn dom.webcomponents.enabled off in bug 1398981

It's off by default on Nightly - I guess you mean it's on for tests, or something?

> and remove the preference in bug 1400762.

I think this is what would need to happen in order for users not to hit bugs in the old implementation.

> Given that, this is very low
> priority for us, not plan to fix it but happy to take a patch.

I think this bug basically depends on bug 1400762, on the assumption that the same issue doesn't happen with the new implementation. If the pref is off by default on nightly, why is removing it difficult?

If we need to keep it around for now for tests, we could rename it so it's more obvious it's old/deprecated. Renaming the pref also has the effect that anyone who may have inadvertently toggled it in the past won't have it toggled on anymore.

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to :Gijs from comment #13)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> > dom.webcomponents.enabled was added for deprecated experimental
> > implementation. We plan to turn dom.webcomponents.enabled off in bug 1398981
> 
> It's off by default on Nightly - I guess you mean it's on for tests, or
> something?

Exactly, it's for tests.

> 
> > and remove the preference in bug 1400762.
> 
> I think this is what would need to happen in order for users not to hit bugs
> in the old implementation.
> 
> > Given that, this is very low
> > priority for us, not plan to fix it but happy to take a patch.
> 
> I think this bug basically depends on bug 1400762, on the assumption that
> the same issue doesn't happen with the new implementation. 

I think setting dependency is a better way. Agreed that this depends on bug 1400762 and should also block Bug 1205323 (shadow DOM v1) as we expect when we have the new implementation ready, we shouldn't hit this issue.

> If the pref is
> off by default on nightly, why is removing it difficult?

Because of tests (see bug 1398981 comment9) and some transition plan from experimental implementation to the new spec'd one as some code will be reused, so we need to have a pref guarded until the new code is ready.

> 
> If we need to keep it around for now for tests, we could rename it so it's
> more obvious it's old/deprecated. Renaming the pref also has the effect that
> anyone who may have inadvertently toggled it in the past won't have it
> toggled on anymore.

Just talked with Jessica, for the work on bug 1398981, she is also considering rename dom.webcomponents.enabled to dom.shadowdom.enabled or something similar. I am looping her here.

Comment 15

[Andrew Overholt [:overholt]]

This seems to work now \o/