1450617 - No need to ResolveTag to disable first-line on fieldsets.

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Emilio Cobos Álvarez (:emilio)]

We don't extend fieldsets using XBL, and I don't think we will.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1450617: No need to ResolveTag to disable first-line on fieldsets.

We don't extend HTML fieldsets with XBL, and I don't think we want to start
doing that now.

Review commit: https://reviewboard.mozilla.org/r/232970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232970/

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8964228 [details]
Bug 1450617: No need to ResolveTag to disable first-line on fieldsets.

https://reviewboard.mozilla.org/r/232970/#review238536

::: layout/base/nsCSSFrameConstructor.cpp:9326
(Diff revision 1)
>  {
>    bool hasFirstLine =
>      nsLayoutUtils::HasPseudoStyle(aContent, aComputedStyle,
>                                    CSSPseudoElementType::firstLine,
>                                    mPresShell->GetPresContext());
> -  if (hasFirstLine) {
> +  if (hasFirstLine && aContent->IsHTMLElement(nsGkAtoms::fieldset)) {

I would be much happier with this if it continued to match the way we determine the FindHTMLData tagname.

With that in mind, how about we change AddFrameConstructionItemsInternal to only consider the return tag and namespace from ResolveTag if the returned namespace is XUL?  You'll presumably need to modify layout/reftests/svg/moz-only/xbl-basic-03.svg and dom/xbl/crashtests/378521-1.xhtml will stop testing anything useful.  But we'd have the invariant that don't need to ever worry about ResolveTag for non-XUL things, at least until we kill ResolveTag altogether.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1450617: Only allow extending from / to XUL elements.

Review commit: https://reviewboard.mozilla.org/r/233218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233218/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1450617: Remove resolved namespace id and tag from FCItems.

Tag is unused, and now that the resolved XBL tag is only XUL the only checks
the namespace id is used for don't need to account for it.

Review commit: https://reviewboard.mozilla.org/r/233220/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233220/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8964228 [details]
Bug 1450617: No need to ResolveTag to disable first-line on fieldsets.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232970/diff/1-2/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> With that in mind, how about we change AddFrameConstructionItemsInternal to
> only consider the return tag and namespace from ResolveTag if the returned
> namespace is XUL?  You'll presumably need to modify
> layout/reftests/svg/moz-only/xbl-basic-03.svg and
> dom/xbl/crashtests/378521-1.xhtml will stop testing anything useful.  But
> we'd have the invariant that don't need to ever worry about ResolveTag for
> non-XUL things, at least until we kill ResolveTag altogether.

Actually surprisingly, xbl-basic-03.svg still passes with these patches, shrug.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8964228 [details]
Bug 1450617: No need to ResolveTag to disable first-line on fieldsets.

https://reviewboard.mozilla.org/r/232970/#review238860

::: layout/base/nsCSSFrameConstructor.cpp:9360
(Diff revision 2)
>  {
>    bool hasFirstLine =
>      nsLayoutUtils::HasPseudoStyle(aContent, aComputedStyle,
>                                    CSSPseudoElementType::firstLine,
>                                    mPresShell->GetPresContext());
> -  if (hasFirstLine) {
> +  if (hasFirstLine && aContent->IsHTMLElement(nsGkAtoms::fieldset)) {

Could just:

    return hasFirstLine && !aContent->IsHTMLElement(nsGkAtoms::fieldset);

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8964483 [details]
Bug 1450617: Only allow extending from / to XUL elements.

https://reviewboard.mozilla.org/r/233218/#review238864

Nice cleanup, thank you!

::: layout/base/nsCSSFrameConstructor.cpp:5703
(Diff revision 1)
>        display = computedStyle->StyleDisplay();
>        aComputedStyle = computedStyle;
> -      aTag = mDocument->BindingManager()->ResolveTag(aContent, &aNameSpaceID);
> +      if (namespaceId == kNameSpaceID_XUL) {
> +        // Only allow overriding from & to XUL.
> +        int32_t overridenNamespace;
> +        nsAtom* overwrittenTag =

"overriddenTag" to be consistent.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8964484 [details]
Bug 1450617: Remove resolved namespace id and tag from FCItems.

https://reviewboard.mozilla.org/r/233220/#review238866

::: layout/base/nsCSSFrameConstructor.h:859
(Diff revision 1)
>                                        nsAtom* aTag,
>                                        int32_t aNameSpaceID,

Can't you remove these args too?

::: layout/base/nsCSSFrameConstructor.h:881
(Diff revision 1)
>                                         nsAtom* aTag,
>                                         int32_t aNameSpaceID,

And these.

::: layout/base/nsCSSFrameConstructor.cpp
(Diff revision 1)
>        new (this) FrameConstructionItem(&sBlockFormattingContextFCData,
>                                  // Use the content of our parent frame
>                                  parentContent,
> -                                // Lie about the tag; it doesn't matter anyway
> -                                pseudoType,
> -                                iter.item().mNameSpaceID,

This is changing behavior, no?  The old namespace was that of the child, but not we will have the namespace of the parent.

::: layout/base/nsCSSFrameConstructor.cpp
(Diff revision 1)
> -                              // Lie about the tag; it doesn't matter anyway
> -                              pseudoType,
> -                              // The namespace does matter, however; it needs
> -                              // to match that of our first child item to
> -                              // match the old behavior
> -                              aIter.item().mNameSpaceID,

Again, this is changing from namespace of child to namespace of parent.  Why is this OK, esp given that comment there?

::: layout/base/nsCSSFrameConstructor.cpp
(Diff revision 1)
>      new (this) FrameConstructionItem(&pseudoData.mFCData,
>                                       // Use the content of the parent frame
>                                       aParentFrame->GetContent(),
> -                                     // Tag type
> -                                     *pseudoData.mPseudoType,
> -                                     // Use the namespace of the rtc frame

And here, changing behavior again.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fe973f9b120
Only allow extending from / to XUL elements. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/b07a4a7694ec
No need to ResolveTag to disable first-line on fieldsets. r=bz

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8964484 [details]
Bug 1450617: Remove resolved namespace id and tag from FCItems.

I'll move this to another bug.

Comment 12

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/5fe973f9b120
https://hg.mozilla.org/mozilla-central/rev/b07a4a7694ec