* SUMMARY Loading the login page for www.flagstar.com doesn't load the CSS for the page, leaving the user with unstyled content (permanently). * STEPS TO REPRODUCE 1. Launch Safari. 2. Open URL: <https://www.flagstar.com/> and select "My Loans" from the "Log into your accounts" dropdown. OR Open URL: <https://www.flagstar.com/myloanswebapp/logonEntry.do?action=Create> * RESULTS Unstyled content. * REGRESSION This is a regression between r84327 (works) and r84329 (fails). Looks like a regression from r84329 for Bug 45425. <http://trac.webkit.org/changeset/84329> * NOTES A follow-up fix was made in Bug 61400 in r88479, but it did not fix all test cases (and WebKit nightly build r91677 still reproduces this). <http://trac.webkit.org/changeset/88479> Bug 62407 was opened to fix the remaining test cases. This bug may be a duplicate of Bug 62407.
<rdar://problem/9835905>
Created attachment 102609 [details] Reduction that needs 2 trivial external stylesheets to work
I started analyzing the bug today and here are my thoughts. * I am not sure if it related to bug 62407 as those 2 failures were already in WebKit, the test just uncovered them (which is why the test landed with those failures in the first place). * The root problem is that we call setDisabled on a <link> whose stylesheet is loading. Currently we just ignore those call as HTMLLinkElement::m_sheet is NULL which is wrong. I will need to look into how Gecko handles that.
Created attachment 102709 [details] Proposed fix: Add a temporary state that we merge back into the stylesheet on sheetLoaded(), also make the code more robust to disabled change.
Comment on attachment 102709 [details] Proposed fix: Add a temporary state that we merge back into the stylesheet on sheetLoaded(), also make the code more robust to disabled change. View in context: https://bugs.webkit.org/attachment.cgi?id=102709&action=review I'm going to say r+ as fixing a regression is good, but it seems like there might be a cleaner solution. Some inconsistent ranting below... > LayoutTests/fast/css/stylesheet-enable-second-alternate.html:2 > +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> > +<html xmlns="http://www.w3.org/1999/xhtml" lang="en" xml:lang="en"> Why all this XHTML boilerplate? > LayoutTests/http/tests/css/resources/slow-loading-sheet.php:3 > +// We sleep here so that we are have enough time to test the different attributes before the stylesheet is fully loaded. > +usleep(100); Sounds flaky. > Source/WebCore/html/HTMLLinkElement.h:115 > + enum PendingSheetDisabledStatus { Unset, Disabled, Enabled }; Do we need three states? From a naive point of view, every sheet is pending enabled unless set otherwise. That would let you use a boolean, improving this rather awkward naming. If that's not accurate, perhaps a comment would help. Also, should the name have "ViaScript" to match m_isEnabledViaScript? In fact, it feels wrong to have these variables separate, as both just remember what was set by a script.
> > LayoutTests/fast/css/stylesheet-enable-second-alternate.html:2 > > +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> > > +<html xmlns="http://www.w3.org/1999/xhtml" lang="en" xml:lang="en"> > > Why all this XHTML boilerplate? I guess I copied the test from an existing one with this flow. Will be removed. > > LayoutTests/http/tests/css/resources/slow-loading-sheet.php:3 > > +// We sleep here so that we are have enough time to test the different attributes before the stylesheet is fully loaded. > > +usleep(100); > > Sounds flaky. It can be indeed. Thought I don't think this is a problem. We just need to test the information prior / after the load to make sure they get updated properly. > > Source/WebCore/html/HTMLLinkElement.h:115 > > + enum PendingSheetDisabledStatus { Unset, Disabled, Enabled }; > > Do we need three states? From a naive point of view, every sheet is pending enabled unless set otherwise. That would let you use a boolean, improving this rather awkward naming. After a quick discussion over IRC, I had a look at removing this tri-state and merging it in the existing boolean m_isEnabledViaScript. I thought that not knowing if JS disabled a style sheet would be a problem (as currently we don't know if false is disabled from JS or just it was never set). However it looks like it is enough for our needs. I will post a patch along those lines shortly!
Created attachment 103326 [details] Proposed fix 2: Less XHTML headers, more reusing m_isEnabledViaScript, more Alexey love?
Created attachment 103566 [details] Proposed fix 3: Even better fix as the previous could lose some information in the way, also tightened the checks between disabled() and m_isEnabledViaScript.
review ping, this patch has been up for review for 7 days and solves some regressions seen in the wild.
Comment on attachment 103566 [details] Proposed fix 3: Even better fix as the previous could lose some information in the way, also tightened the checks between disabled() and m_isEnabledViaScript. View in context: https://bugs.webkit.org/attachment.cgi?id=103566&action=review The logic is pretty convoluted and hard to follow. Is that the case with the spec as well? r- for all the style issues and open questions. > Source/WebCore/html/HTMLLinkElement.cpp:84 > void HTMLLinkElement::setDisabled(bool disabled) > { The code seems to assume this is only ever invoked from script, via DOM. It would be nice to assert that somehow. > Source/WebCore/html/HTMLLinkElement.cpp:89 > + // If we are in the middle of loading a stylesheet, we store the information from this call > + // as it is a common pattern to disable / enable the stylesheet through JS (regardless of > + // whether the sheet was loaded). > + // See https://bugs.webkit.org/show_bug.cgi?id=65140 The verbose comment here adds little information. We don't generally link to bugzilla on every code change. > Source/WebCore/html/HTMLLinkElement.cpp:91 > + if (m_relAttribute.m_isStyleSheet && m_loading) > + setEnabledViaScript(!disabled); Why these specific conditions? Can't non-stylesheet links be disabled? Why only when loading? setDisabled() does not have these tests in other branches. Could the setEnabledViaScript call simply be moved to the top of this function? It seems to be called in all cases, except for these tests. > Source/WebCore/html/HTMLLinkElement.cpp:99 > + if (wasDisabled == disabled) { > + // If JS forces the sheet to be enabled, we need to force a recalcStyleSelector or > + // we may not apply the stylesheet (for example for alternate stylesheet). I don't quite understand why we need to take some special action in a case where nothing supposedly changes? > Source/WebCore/html/HTMLLinkElement.cpp:100 > + if (isEnabledViaScript() != !disabled) { Triple negatives make my head hurt. > Source/WebCore/html/HTMLLinkElement.cpp:102 > + setEnabledViaScript(!disabled); > + document()->styleSelectorChanged(DeferRecalcStyle); If changing the enabled view script state requires style recalc, would it better to trigger it in that function itself? > Source/WebCore/html/HTMLLinkElement.cpp:474 > + if (m_sheet) > + return m_sheet->disabled(); Please handle the special !m_sheet case as early return and the regular case at the end of the function. > Source/WebCore/html/HTMLLinkElement.cpp:479 > + // FF disagrees with the spec and always has an associated stylesheet if the 'rel' attribute indicates a > + // stylesheet and href is provided (regardless of whether the resource may be in error). As we store the > + // enabled state in m_isEnabledViaScript while loading, return this information to be consistent with FF > + // and our future self here. Do you mean that we violate the spec here to be compatible with FF? Why? What does the spec say? Our future self? I don't understand the comment. > Source/WebCore/html/HTMLLinkElement.h:60 > - bool isEnabledViaScript() const { return m_isEnabledViaScript; } > + bool isEnabledViaScript() const { checkDisabledAndEnabledViaScriptState(); return m_isEnabledViaScript == Enabled; } Since the API is setDisabled, reversing the logic might make some of the code read better (isDisabledViaScript()) > Source/WebCore/html/HTMLLinkElement.h:61 > + void setEnabledViaScript(bool enabled) { m_isEnabledViaScript = enabled ? Enabled : Disabled; } Why is the setter public? Since the API is setDisabled, reversing the logic might make some of the code read better. > Source/WebCore/html/HTMLLinkElement.h:104 > + void checkDisabledAndEnabledViaScriptState() const > + { > + // When we are loading, m_isEnabledViaScript stores the information if JS calls us and can thus have any value. > + // If m_isEnabledViaScript is Unset, the style sheet may be unabled or disabled. > + // Finally if we are not in the 2 previous cases, then JS should override our disabled value so we must make sure > + // that both values match. > + ASSERT(isLoading() || m_isEnabledViaScript == Unset || ((m_isEnabledViaScript == Disabled && sheet()->disabled()) || (m_isEnabledViaScript == Enabled && !sheet()->disabled()))); > + } Confusingly this function is a pure assertion. It would be better to replace it with a boolean debug-only function (isDisabledStateConsistent() or something) and ASSERT on that instead. The comment is overly verbose and adds little value. > Source/WebCore/html/HTMLLinkElement.h:118 > - bool m_isEnabledViaScript; > + enum EnabledViaScriptState { Unset, Enabled, Disabled }; > + EnabledViaScriptState m_isEnabledViaScript; m_is* naming is for booleans. Something like m_scriptState might be good, with values like Unset/EnabledViaScript/DisabledViaScript.
Comment on attachment 103566 [details] Proposed fix 3: Even better fix as the previous could lose some information in the way, also tightened the checks between disabled() and m_isEnabledViaScript. View in context: https://bugs.webkit.org/attachment.cgi?id=103566&action=review >> Source/WebCore/html/HTMLLinkElement.cpp:84 >> { > > The code seems to assume this is only ever invoked from script, via DOM. It would be nice to assert that somehow. I agree but I don't think we can do that easily. One way would be to create custom wrappers which opens its own can of worms. >> Source/WebCore/html/HTMLLinkElement.cpp:89 >> + // See https://bugs.webkit.org/show_bug.cgi?id=65140 > > The verbose comment here adds little information. We don't generally link to bugzilla on every code change. It is not uncommon to link to the bug to explain why it is needed. In this case, it links back to this bug for context. >> Source/WebCore/html/HTMLLinkElement.cpp:91 >> + setEnabledViaScript(!disabled); > > Why these specific conditions? Can't non-stylesheet links be disabled? Why only when loading? setDisabled() does not have these tests in other branches. Per HTML5 standard: "The IDL attribute disabled only applies to style sheet links. When the link element defines a style sheet link, then the disabled attribute behaves as defined for the alternative style sheets DOM. For all other link elements it always return false and does nothing on setting." Unfortunately we don't implement the filtering at the binding level nor anywhere else. That's another bug thought which should not impact this change. Let's just remove the checks. > Could the setEnabledViaScript call simply be moved to the top of this function? It seems to be called in all cases, except for these tests. It looks like it is possible to move setEnabledViaScript. >> Source/WebCore/html/HTMLLinkElement.cpp:99 >> + // we may not apply the stylesheet (for example for alternate stylesheet). > > I don't quite understand why we need to take some special action in a case where nothing supposedly changes? Currently we make a difference between an enabled stylesheet and a stylesheet enabled through JS. To be more precise, alternate style sheet are applied only if they were enabled through JS. This matches the spec and we don't want to change that. >> Source/WebCore/html/HTMLLinkElement.cpp:102 >> + document()->styleSelectorChanged(DeferRecalcStyle); > > If changing the enabled view script state requires style recalc, would it better to trigger it in that function itself? Good point, I will change the logic to move the styleSelectorChanged to the setter. >> Source/WebCore/html/HTMLLinkElement.cpp:479 >> + // and our future self here. > > Do you mean that we violate the spec here to be compatible with FF? Why? What does the spec say? > > Our future self? I don't understand the comment. No, we don't violate the spec, it's FF who does: basically it states that you create your stylesheet only when it was fetched from the network (CSS OM: "6.3.4 Requirements on User Agents Implementing the HTTP Link Header"). As for the future self, it meant when we would reconcile the temporary state with the disabled state from the stylesheet. >> Source/WebCore/html/HTMLLinkElement.h:60 >> + bool isEnabledViaScript() const { checkDisabledAndEnabledViaScriptState(); return m_isEnabledViaScript == Enabled; } > > Since the API is setDisabled, reversing the logic might make some of the code read better (isDisabledViaScript()) But it would make the recalcStyleSelector logic not as good. We are interested here whether we forced the stylesheet to be enabled. >> Source/WebCore/html/HTMLLinkElement.h:61 >> + void setEnabledViaScript(bool enabled) { m_isEnabledViaScript = enabled ? Enabled : Disabled; } > > Why is the setter public? Since the API is setDisabled, reversing the logic might make some of the code read better. Same comment as above. It should not be public though. >> Source/WebCore/html/HTMLLinkElement.h:104 >> + } > > Confusingly this function is a pure assertion. It would be better to replace it with a boolean debug-only function (isDisabledStateConsistent() or something) and ASSERT on that instead. > > The comment is overly verbose and adds little value. Fair enough, I will change the method and remove the comment. >> Source/WebCore/html/HTMLLinkElement.h:118 >> + EnabledViaScriptState m_isEnabledViaScript; > > m_is* naming is for booleans. Something like m_scriptState might be good, with values like Unset/EnabledViaScript/DisabledViaScript. I am not too keen on m_scriptState but the alternatives I came with are not any better.
Created attachment 104415 [details] New version 4: Simplified further the code after Anti's comments.
Created attachment 104416 [details] New version 4: Simplified further the code after Antti's comments (sorry for the mispelling).
>> If changing the enabled view script state requires style recalc, would it better to trigger it in that function itself? > Good point, I will change the logic to move the styleSelectorChanged to the setter. FYI I did not do this change as we would end up doing unneeded style recalc. If you don't have a stylesheet and are just storing the information temporary in m_scriptState, you shouldn't do a recalcStyleSelector. Same if you are following the old code path where this will be called at the right time (during process()). Instead I kept the manual call as it was the only one needed. Let me know what you think.
Comment on attachment 104416 [details] New version 4: Simplified further the code after Antti's comments (sorry for the mispelling). After discussing the change over IRC, it became clear there should be a test for an alternate stylesheet in error to make sure we are consistent in this case. Clearing the review until I come up with such a test.
Created attachment 104520 [details] New version 5: Tweaked the debug-only code + added testing for an alternate stylesheet in error.
Comment on attachment 104520 [details] New version 5: Tweaked the debug-only code + added testing for an alternate stylesheet in error. r=me
Created attachment 104526 [details] Patch for landing
Comment on attachment 104526 [details] Patch for landing Clearing flags on attachment: 104526 Committed r93425: <http://trac.webkit.org/changeset/93425>
All reviewed patches have been landed. Closing bug.
Rolled out the change in http://trac.webkit.org/changeset/93439 as the ASSERT would trigger on some of our buildbots.
Created attachment 104613 [details] Patch 6: Solve the valid ASSERT by adding a custom setter to |disabled| that forwards the information through the associated <link> (if any).
So now setting disabled attribute on stylesheet also changes the disabled value of the referencing link element? Does this match spec and other browsers? Test case?
(In reply to comment #23) > So now setting disabled attribute on stylesheet also changes the disabled value of the referencing link element? It's more than that. <link>'s disabled is basically its sheet's disabled value if it has one. If not, it returns false on getting and does nothing on setting (this last point is what caused the regression as we were too strict). > Does this match spec and other browsers? Test case? It matches HTML5 that defines the behavior of <link>'s disabled. FF also implements that. The forwarding is already covered by LayoutTests/fast/dom/HTMLLinkElement/disabled-attribute.html (see bug 45425 which landed and caused this regression). However you have a point, the tests that comes with this patch do not cover setting sheet.disabled enough. I will add more testing on this side.
Created attachment 104686 [details] Patch 7: Same as previously but with more testing for the sheet.disabled() case.
Actually from my reading this patch breaks the HTML5 spec: http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#styling "... link elements whose specified resource has not yet been fetched, ... , must have their LinkStyle interface's sheet attribute return null." and "The disabled IDL attribute on link and style elements must return false and do nothing on setting, if the sheet attribute of their LinkStyle interface is null." According to these the disabled attribute must do nothing as long as the resource has not been fetched.
> According to these the disabled attribute must do nothing as long as the resource has not been fetched. Yes, however that's exactly what caused this regression. If we ignore JS during loading then we end up missing what web-sites are currently doing. FF does not have this regression because it always has an associated stylesheet for that matter (which violates HTML5 as you just pointed out). Note that until we strictly implemented HTML5, we did not have any of those problems as disabled would work in any cases.
(In reply to comment #27) > Yes, however that's exactly what caused this regression. If we ignore JS during loading then we end up missing what web-sites are currently doing. > FF does not have this regression because it always has an associated stylesheet for that matter (which violates HTML5 as you just pointed out). Note that until we strictly implemented HTML5, we did not have any of those problems as disabled would work in any cases. I tried to ask you exactly this above: "Do you mean that we violate the spec here to be compatible with FF?" You answered "No, we don't violate the spec, it's FF who does" which sounded to me different from what you are now saying. Are there other known regressions from this? If not perhaps we should stick with the HTML5 and try to get the site fixed. If we want to have this behavior, we also need to get the specification changed. What do IE9 and Opera do?
(In reply to comment #28) > (In reply to comment #27) > > > Yes, however that's exactly what caused this regression. If we ignore JS during loading then we end up missing what web-sites are currently doing. > > > FF does not have this regression because it always has an associated stylesheet for that matter (which violates HTML5 as you just pointed out). Note that until we strictly implemented HTML5, we did not have any of those problems as disabled would work in any cases. > > I tried to ask you exactly this above: "Do you mean that we violate the spec here to be compatible with FF?" You answered "No, we don't violate the spec, it's FF who does" which sounded to me different from what you are now saying. I misunderstood the "to be compatible" as "to match FF" (which we don't) and polarized my answer on that, which was indeed wrong. Sorry for the misunderstanding. The proper answer should be: this patch and FF are violating the specifications in 2 different ways. > Are there other known regressions from this? Yes, board.4chan.org is also impacted (http://code.google.com/p/chromium/issues/detail?id=84300). > If not perhaps we should stick with the HTML5 and try to get the site fixed. We could but I don't think such regressions are good to keep. > If we want to have this behavior, we also need to get the specification > changed. What do IE9 and Opera do? IE9 does not support link.sheet. It abides by link.disabled as if there were always a stylesheet. Opera matches FF and always had an associated stylesheet. Poking further into the behavior of both browsers, it looks like: * They both fill link.sheet with default values even if the stylesheet was in error. * FF loads any stylesheet synchronously - at least from a JS perspective. * Opera does asynchronous loading for alternate sheets (thus querying the sheet can lead to different results if you reload the page). The results are consistent within one load though (loading vs loaded sheet)
So this gives us behavior that is a) against the spec b) does not really match any other engine. This does not seem ideal. Is there some good reason we don't just revert http://trac.webkit.org/changeset/84329 and go back to our original behavior (which is also against spec and doesn't match other engines but has distinct advantage of being hundreds of lines less code)?
When you intentionally implement something that violates the spec, the spec is wrong. Please always let me know when the spec is wrong! I recommend posting to whatwg@whatwg.org so that other browser vendors can discuss what the best plan of action is here. What does IE do? What does Firefox do? If they're the same, that's what the spec should say here.
> Is there some good reason we don't just revert http://trac.webkit.org/changeset/84329 and go back to our original behavior (which is also against spec and doesn't match other engines but has distinct advantage of being hundreds of lines less code)? I have no objections to reverting 84329 and the follow-up fix as those changes have lead to several regressions. However we need to keep the test coverage developed while investigating those regressions though. As Ian pointed out, we have to bring the behavior to the WHATWG. I was thinking of doing that as a post-mortem of those regressions.
(In reply to comment #32) > > Is there some good reason we don't just revert http://trac.webkit.org/changeset/84329 and go back to our original behavior (which is also against spec and doesn't match other engines but has distinct advantage of being hundreds of lines less code)? > > I have no objections to reverting 84329 and the follow-up fix as those changes have lead to several regressions. However we need to keep the test coverage developed while investigating those regressions though. > > As Ian pointed out, we have to bring the behavior to the WHATWG. I was thinking of doing that as a post-mortem of those regressions. Yeah, at this point I think it would be good to revert the code changes for now (but keep the tests) while we figure out what we exactly want to do. The spec clearly needs to change (which is why I mentioned it above " … we also need to get the specification changed." :)
Created attachment 106028 [details] Patch 8: Revert the 2 previous changes, keeping the test cases developped (covered by 67436).
Comment on attachment 106028 [details] Patch 8: Revert the 2 previous changes, keeping the test cases developped (covered by 67436). Clearing flags on attachment: 106028 Committed r94369: <http://trac.webkit.org/changeset/94369>
Sounds like this bug shouldn't be closed.
> Sounds like this bug shouldn't be closed. Why? It looks like the problem with flagstar.com got fixed.
(In reply to comment #38) > > Sounds like this bug shouldn't be closed. > > Why? It looks like the problem with flagstar.com got fixed. I misunderstood what was being reverted in r94369. My mistake.