Commit d83e7a60 authored by Masayuki Nakano's avatar Masayuki Nakano
Browse files

Bug 1808906 - Make the style editor handle `<font>` at last when there are... 

Bug 1808906 - Make the style editor handle `<font>` at last when there are multiple preserved styles r=m_kato

Height of inline elements are considered with current `font-size` and
`line-height`.  Therefore, if content in inline elements are taller, the
`background-color` does not fill the bigger content background entirely.
For solving this issue, Chrome handles styles of `<font>` element as
outer-most style.  This is reasonable approach, let's follow this.

For solving this issue, we can change the order of `PreservedStyle`s at setting
the preserved styles.  Then, `SetInlinePropertiesAsSubAction` is called with
reversed order and apply later style first and applies newer styles to all
content in an element which is previously inserted.  Therefore, the `<font>`
element styles should be last elements of `PendingStyles::mPreservingStyles`.

When applying new style, our style editor does not reuse existing `<font>`
element, and this causes writing WPT harder.  Therefore, this patch also changes
the applying range of `<font>` style to wrapping existing `<font>` element if
and only if its content is entirely selected.

Unfortunately, this approach cannot get exactly same result as Chrome because we
insert outer-most `<font>` element first, then, try to apply `background-color`,
at this moment, our style editor applies the style to the previously inserted
`<font>` element instead of creating new `<span>` element.  This behavior is
required for compatibility in the other cases.  Additionally, changing only this
behavior requires a lot of method changes to specify how to handle it.  However,
this incompatible behavior may not cause any problems in web apps in the wild.
Therefore, this patch does not solve this incompatible issue.  I think that once
we get a bug report caused by this difference, we should redesign how to set
multiple inline styles once.

Depends on D166416

Differential Revision: https://phabricator.services.mozilla.com/D166617
parent bf6597cc

editor/libeditor/HTMLEditHelpers.h

+11 −0
Original line number Diff line number Diff line
@@ -994,6 +994,17 @@ struct MOZ_STACK_CLASS EditorInlineStyle : public EditorElementStyle { 
            mHTMLProperty == nsGkAtoms::s);

  }



  /**

   * Returns true if the style can be represented with <font>.

   */

  [[nodiscard]] bool IsStyleOfFontElement() const {

    MOZ_ASSERT_IF(

        mHTMLProperty == nsGkAtoms::font,

        mAttribute == nsGkAtoms::bgcolor || mAttribute == nsGkAtoms::color ||

            mAttribute == nsGkAtoms::face || mAttribute == nsGkAtoms::size);

    return mHTMLProperty == nsGkAtoms::font && mAttribute != nsGkAtoms::bgcolor;

  }



  /**

   * Returns true if the style is conflict with vertical-align even though

   * they are not mapped to vertical-align in the CSS mode.

editor/libeditor/HTMLEditSubActionHandler.cpp

+3 −0
Original line number Diff line number Diff line
@@ -6172,6 +6172,9 @@ Result<EditorDOMPoint, nsresult> HTMLEditor::CreateStyleForInsertText( 
    insertNewTextNodeResult.inspect().IgnoreCaretPointSuggestion();

    pointToPutCaret.Set(newEmptyTextNode, 0u);



    // FIXME: If the stylesToSet have background-color style, it may

    // be applied shorter because outer <span> element height is not

    // computed with inner element's height.

    HTMLEditor::FontSize incrementOrDecrement =

        relFontSize > 0 ? HTMLEditor::FontSize::incr

                        : HTMLEditor::FontSize::decr;

editor/libeditor/HTMLStyleEditor.cpp

+16 −0
Original line number Diff line number Diff line
@@ -1601,6 +1601,8 @@ EditorRawDOMPoint HTMLEditor::AutoInlineStyleSetter:: 
    return startPoint;

  }



  const bool useCSS = aHTMLEditor.IsCSSEnabled();

  const bool isFontElementStyle = IsStyleOfFontElement();

  Element* mostDistantStartParentHavingStyle = nullptr;

  for (Element* parent :

       startPoint.GetContainer()->InclusiveAncestorsOfType<Element>()) {
@@ -1612,6 +1614,12 @@ EditorRawDOMPoint HTMLEditor::AutoInlineStyleSetter:: 
    if (ContentIsElementSettingTheStyle(aHTMLEditor, *parent)) {

      mostDistantStartParentHavingStyle = parent;

    }

    // If we're setting <font> element and there is a <font> element which is

    // entirely selected, we should use it.

    else if (!useCSS && isFontElementStyle &&

             parent->IsHTMLElement(nsGkAtoms::font)) {

      mostDistantStartParentHavingStyle = parent;

    }

    if (parent->GetPreviousSibling()) {

      break;  // The parent is not first element in its parent, stop climbing.

    }
@@ -1633,6 +1641,8 @@ EditorRawDOMPoint HTMLEditor::AutoInlineStyleSetter:: 
    return endPoint;

  }



  const bool useCSS = aHTMLEditor.IsCSSEnabled();

  const bool isFontElementStyle = IsStyleOfFontElement();

  Element* mostDistantEndParentHavingStyle = nullptr;

  for (Element* parent :

       endPoint.GetContainer()->InclusiveAncestorsOfType<Element>()) {
@@ -1644,6 +1654,12 @@ EditorRawDOMPoint HTMLEditor::AutoInlineStyleSetter:: 
    if (ContentIsElementSettingTheStyle(aHTMLEditor, *parent)) {

      mostDistantEndParentHavingStyle = parent;

    }

    // If we're setting <font> element and there is a <font> element which is

    // entirely selected, we should use it.

    else if (!useCSS && isFontElementStyle &&

             parent->IsHTMLElement(nsGkAtoms::font)) {

      mostDistantEndParentHavingStyle = parent;

    }

    if (parent->GetNextSibling()) {

      break;  // The parent is not last element in its parent, stop climbing.

    }

editor/libeditor/PendingStyles.cpp

+23 −2
Original line number Diff line number Diff line
@@ -373,8 +373,29 @@ void PendingStyles::PreserveStyle(nsStaticAtom& aHTMLProperty, 
    return;

  }



  mPreservingStyles.AppendElement(MakeUnique<PendingStyle>(

      &aHTMLProperty, aAttribute, aAttributeValueOrCSSValue));

  // font-size and font-family need to be applied outer-most because height of

  // outer inline elements of them are computed without these styles.  E.g.,

  // background-color may be applied bottom-half of the text.  Therefore, we

  // need to apply the font styles first.

  uint32_t fontStyleCount = 0;

  for (const UniquePtr<PendingStyle>& style : Reversed(mPreservingStyles)) {

    if (style->GetTag() != nsGkAtoms::font ||

        style->GetAttribute() == nsGkAtoms::bgcolor) {

      break;

    }

    MOZ_ASSERT(style->GetAttribute() == nsGkAtoms::color ||

               style->GetAttribute() == nsGkAtoms::face ||

               style->GetAttribute() == nsGkAtoms::size);

    fontStyleCount++;

  }

  UniquePtr<PendingStyle> style = MakeUnique<PendingStyle>(

      &aHTMLProperty, aAttribute, aAttributeValueOrCSSValue);

  if (fontStyleCount) {

    mPreservingStyles.InsertElementAt(

        mPreservingStyles.Length() - fontStyleCount, std::move(style));

  } else {

    mPreservingStyles.AppendElement(std::move(style));

  }



  CancelClearingStyle(aHTMLProperty, aAttribute);

}

testing/web-platform/tests/editing/data/fontname.js

+46 −1
Original line number Diff line number Diff line
@@ -704,5 +704,50 @@ var browserTests = [ 
    [["stylewithcss","false"],["fontname","sans-serif"]],

    "<span style=\"font-family:monospace\">fo<font face=\"sans-serif\">[o</font></span><kbd><font face=\"sans-serif\">b]</font>ar</kbd>",

    [true,true],

    {"stylewithcss":[false,true,"",false,false,""],"fontname":[false,false,"monospace",false,false,"sans-serif"]}]

    {"stylewithcss":[false,true,"",false,false,""],"fontname":[false,false,"monospace",false,false,"sans-serif"]}],



// If contents of <font> are entirely selected, it should be reused.

["<font size=7>[abc]</font>",

   [["styleWithCSS","false"],["fontName","monospace"]],

   ["<font face=\"monospace\" size=\"7\">[abc]</font>",

    "<font size=\"7\" face=\"monospace\">[abc]</font>"],

   [true,true],

   {}],

["<font color=#ff0000>[abc]</font>",

   [["styleWithCSS","false"],["fontName","monospace"]],

   ["<font face=\"monospace\" color=\"#ff0000\">[abc]</font>",

    "<font color=\"#ff0000\" face=\"monospace\">[abc]</font>"],

   [true,true],

   {}],

["<font size=\"7\" color=#ff0000>[abc]</font>",

   [["styleWithCSS","false"],["fontName","monospace"]],

   ["<font color=\"#ff0000\" face=\"monospace\" size=\"7\">[abc]</font>",

    "<font color=\"#ff0000\" size=\"7\" face=\"monospace\">[abc]</font>",

    "<font face=\"monospace\" color=\"#ff0000\" size=\"7\">[abc]</font>",

    "<font face=\"monospace\" size=\"7\" color=\"#ff0000\">[abc]</font>",

    "<font size=\"7\" color=\"#ff0000\" face=\"monospace\">[abc]</font>",

    "<font size=\"7\" face=\"monospace\" color=\"#ff0000\">[abc]</font>"],

   [true,true],

   {}],

// but don't split existing <font> if partially selected.

["<font size=7>[a]bc</font>",

   [["styleWithCSS","false"],["fontName","monospace"]],

   "<font size=\"7\"><font face=\"monospace\">[a]</font>bc</font>",

   [true,true],

   {}],

["<font size=7>ab[c]</font>",

   [["styleWithCSS","false"],["fontName","monospace"]],

   "<font size=\"7\">ab<font face=\"monospace\">[c]</font></font>",

   [true,true],

   {}],

["<font color=#ff0000>[a]bc</font>",

   [["styleWithCSS","false"],["fontName","monospace"]],

   "<font color=\"#ff0000\"><font face=\"monospace\">[a]</font>bc</font>",

   [true,true],

   {}],

["<font color=#ff0000>ab[c]</font>",

   [["styleWithCSS","false"],["fontName","monospace"]],

   "<font color=\"#ff0000\">ab<font face=\"monospace\">[c]</font></font>",

   [true,true],

   {}],

]