745131 - (semantics2) Improve how <semantics> determine the visible child

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[Frédéric Wang (:fredw)]

Follow-up of bug 154931 and bug 556767. Our current implementation is described in mathml.css:

-----
/* Hide embedded semantic MathML content (as opposed to presentational
   content, which we render). Ideally, here is the behavior that we want:

   if there is an annotation-xml[encoding="MathML-Presentation"]
     render that annotation, and ignore the first child of the
     <semantics> element and all other annotations, 
   else
     render the first child of <semantics> and ignore all annotations

   But this cannot be expressed with CSS. As a stop-gap, just render
   the first child to cater for most of the common cases - bug 154931.
*/
semantics > :not(:first-child) {
  display: none;
}
-----

The purpose of this bug is to implement the intended behavior. I think this will be similar to the maction case and so we can share the implementation. We can probably create a class nsMathMLselectedFrame that inherits from nsMathMLContainerFrame, overrides

nsMathMLContainerFrame::ChildListChanged
nsMathMLContainerFrame::BuildDisplayList
nsMathMLContainerFrame::Place

and implements a default

nsMathMLselectedFrame::GetSelectedFrame function (say, which returns the first child if any).

Then nsMathMLmactionFrame and nsMathMLsemanticsFrame will override nsMathMLselectedFrame::GetSelectedFrame. In nsMathMLsemanticsFrame that will be browsing the child list to find an annotation-xml element with encoding attribute "application/mathml-presentation+xml" (or "MathML-Presentation" for backward compatibility) and default to the first child otherwise.

Comment 1

[Frédéric Wang (:fredw)]

Gecko may also try to display the first <annotation-xml> with a known encoding (presentation MathML, SVG, HTML) or text <annotation>. This will avoid to break Jacques Distler testcase in

https://bugs.webkit.org/show_bug.cgi?id=100626

Comment 2

[Frédéric Wang (:fredw)]

Attachment: testcase

Comment 3

[Frédéric Wang (:fredw)]

Attachment: Patch V1

So here is a first patch that tries to reproduce the behavior discussed elsewhere. Still a WIP.

Comment 4

[Frédéric Wang (:fredw)]

Bill: could would please include this patch in your builds, so that people could give their advice on the patch behavior.

Comment 5

[Bill Gianopoulos [:WG9s]]

(In reply to Frédéric Wang (:fredw) from comment #4)
> Bill: could would please include this patch in your builds, so that people
> could give their advice on the patch behavior.

I tried yet the nsMathMLmactionFrame.cpp and nsMathMLmactionFrame.h portions of the patch did not apply even with -F7.  Is there some other patch i need to apply first to get this to apply as posted?

Comment 6

[Frédéric Wang (:fredw)]

I don't know if Bill solved his issue with "hg copy" but anyway I've sent my patches to the try server. Builds should be available here (for the moment the directory is not created, so you'll get a 404 error):

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-beae9064e260

Comment 7

[Frédéric Wang (:fredw)]

Attachment: C++ source code of the GetSelectedFrame function

Here is the important part of the <semantics> implementation: the selection of the visible frame. Please tell me if that sounds reasonable to you. I guess you all read C++ and I've commented the code, but tell me if you need precision.

I don't think Webkit implements space-like or embellished op but basically the rules for <semantics> will be the same as <maction> and <annotation-xml> will behave as an <mrow>.

Comment 8

[Frédéric Wang (:fredw)]

Attachment: Patch V2

Comment 9

[Frédéric Wang (:fredw)]

Attachment: Reftests V1

Comment 10

[Frédéric Wang (:fredw)]

There were so assertions due to <annotation> trying to find the direction attribute, but this should be fixed now.

https://tbpl.mozilla.org/?tree=Try&rev=beae9064e260

The builds should now be available here:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-beae9064e260

Comment 11

[Frédéric Wang (:fredw)]

The tests below can be considered to pass with the patches attached to this bug:

http://www.w3.org/Math/testsuite/build/main/Presentation/CSS/semantics/semantics-01-full.xhtml
http://www.w3.org/Math/testsuite/build/main/Presentation/CSS/semantics/semantics-02-full.xhtml
http://www.w3.org/Math/testsuite/build/main/Presentation/CSS/semantics/semantics-03-full.xhtml
http://www.w3.org/Math/testsuite/build/main/Presentation/CSS/semantics/semantics-04-full.xhtml

http://www.w3.org/Math/testsuite/build/main/Content/SemanticMappingElements/annotation/rec-annotation1-full.xhtml
http://www.w3.org/Math/testsuite/build/main/Content/SemanticMappingElements/semantics/rec-semantics1-full.xhtml

In addition to release notes, we will need to update

https://developer.mozilla.org/en-US/docs/Mozilla_MathML_Project/MathML3Testsuite
https://developer.mozilla.org/en-US/docs/Mozilla_MathML_Project/Status
https://developer.mozilla.org/en-US/docs/MathML/Element#Other_elements

Comment 12

[Bill Gianopoulos [:WG9s]]

(In reply to Frédéric Wang (:fredw) from comment #6)
> I don't know if Bill solved his issue with "hg copy" but anyway I've sent my
> patches to the try server. Builds should be available here (for the moment
> the directory is not created, so you'll get a 404 error):

I finally remembered how my script to process copies worked.  Builds are in progress now.

Comment 13

[Frédéric Wang (:fredw)]

Karl: any update on the review?

Comment 14

[Bill Gianopoulos [:WG9s]]

(In reply to Frédéric Wang (:fredw) from comment #8)
> Created attachment 677519 [details] [diff] [review]
> Patch V2

This patch no longer applies cleanly.

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 677520 [details] [diff] [review]
Reftests V1

I wonder why "content MathML" in semantics-2 compares semantics with maction.
Be careful not to rely on tests passing because the implementation is shared.
Is content MathML valid within maction?

"annotation 2" may actually be a case for preferring known encodings over 
an annotation assumed to be plain text, but simple rules are easier for
developers to predict.

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 677519 [details] [diff] [review]
Patch V2

This approach looks sensible to me.  A few things with the interaction of base
and derived classes could be improved to make things clearer, I think.

A nsMathMLSelectedFrame is never instantiated except as a derived class.  I
assume that means that NS_IMPL_FRAMEARENA_HELPERS and FRAME_ID are not
required for this class.  Can you make the build succeed without these
declarations?

Similarly nsMathMLSelectedFrame::GetSelectedFrame() can be pure virtual.

Moving nsMathMLSelectedFrame::SetInitialChildList() to nsMathMLsemanticsFrame
would make it clearer that this method is only used there.
Or perhaps nsMathMLmactionFrame::SetInitialChildList() could first call the
base class method and check mSelectedFrame where it currently uses the
GetSelectedFrame() return value.
Actually, there is an inconsistency between SetInitialChildList setting
mActionType to none when there are no children but ChildListChanged doesn't.
I suspect SetInitialChildList doesn't need to set mActionType to none.
nsMathMLmactionFrame::SetInitialChildList() could always add the event
listeners, or I guess that could be done in Init() and nsMathMLmactionFrame
would just use the base class SetInitialChildList()

nsMathMLmactionFrame::ChildListChanged() and
nsMathMLSelectedFrame::ChildListChanged() both call GetSelectedFrame(), even
though one chains up to the other.

>-nsMathMLmactionFrame::ChildListChanged(int32_t aModType)
>+nsMathMLSelectedFrame::ChildListChanged(int32_t aModType)
> {
>   // update cached values
>-  mChildCount = -1;
>-  mSelection = 0;
>+  mInvalidMarkup = false;
>   mSelectedFrame = nullptr;
>   GetSelectedFrame();

nsMathMLsemanticsFrame doesn't use cached values, so I think it would
make more sense to leave "mSelectedFrame = nullptr" in nsMathMLmactionFrame
and remove the comment here.

nsMathMLmactionFrame::GetSelectedFrame() always sets mInvalidMarkup
appropriately (if it has changed), so I think it would make sense to do
similarly in nsMathMLsemanticsFrame::GetSelectedFrame() and then
mInvalidMarkup would not need to be reset here.

>-  // This very first call to GetSelectedFrame() will cause us to be marked as an
>-  // embellished operator if the selected child is an embellished operator

Should this comment be retained in nsMathMLSelectedFrame (because it also
applies to semantics)?

>+  //   - <semantics-xml> behaves the same as <mrow>

"annotation-xml", I assume.

>+  // Using <annotation> or <annotation-xml> as as a first child is invalid.

Remove one "as".

>+  bool firstChildIsAnnotation = false;
>+  if (childFrame) {

childFrame was checked above, so no need to check again here.

Removing that test mean that the next if block can be replaced with
initialization of firstChildIsAnnotation with the boolean expression.

>+  // If the first child is a presentation MathML element other than
>+  // <annotation> or <annotation-xml>, we are done.
>+  if (!firstChildIsAnnotation &&
>+      childFrame && childFrame->IsFrameOfType(nsIFrame::eMathML)) {

Similarly, here no need to check childFrame. 

>+        // If the <annotation> element has an src attribute, it is a reference
>+        // and we ignore it. Otherwise, we assume it is a plain text annotation

Leave out "plain" from this comment, I assume.  I expect application/x-tex
might be a common encoding here.

annotation-xml elements can also have a src attribute.  Should they also be
ignored for now, or are they actually fetched?

Consider using |continue| to move to the next element in this loop, requiring fewer levels of indent.

Comment 17

[Frédéric Wang (:fredw)]

Thanks Karl. I'm clearing the "assign to" field for now and setting this bug as mentored, so that someone might finish the work.

(In reply to Karl Tomlinson (:karlt) from comment #15)
> Comment on attachment 677520 [details] [diff] [review]
> Reftests V1
> 
> I wonder why "content MathML" in semantics-2 compares semantics with maction.
> Be careful not to rely on tests passing because the implementation is shared.
> Is content MathML valid within maction?

Yes, I think it should just be <csymbol>. Perhaps I did that to prevent some pixel diff but I don't remember.

> 
> "annotation 2" may actually be a case for preferring known encodings over 
> an annotation assumed to be plain text, but simple rules are easier for
> developers to predict.

I guess you mean that application/mathml-presentation+xml could be chosen here. In this implementation, I'm taken the simplest approach and display the first known encoding.


(In reply to Karl Tomlinson (:karlt) from comment #16)
> >-  // This very first call to GetSelectedFrame() will cause us to be marked as an
> >-  // embellished operator if the selected child is an embellished operator
> 
> Should this comment be retained in nsMathMLSelectedFrame (because it also
> applies to semantics)?

I think I reported this to the Math WG mailing list. More generally, Neil proposed to take the embellished op data from the first embellished op child. Again, I prefer the simpler approach to take the embellished op data from the selected child and this is consistent with maction.

> annotation-xml elements can also have a src attribute.  Should they also be
> ignored for now, or are they actually fetched?

I think they should be ignored too for now.

Comment 18

[Frédéric Wang (:fredw)]

Attachment: Patch V3 - part 1

Comment 19

[Frédéric Wang (:fredw)]

Attachment: Patch V3 - part 2

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 736783 [details] [diff] [review]
Patch V3 - part 1

The changes are good.  The r- is just because this patch shouldn't be committed on its own as this revision won't build due to duplicate symbols.

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 736784 [details] [diff] [review]
Patch V3 - part 2

Missed this from comment 16:

> nsMathMLmactionFrame::ChildListChanged() and
> nsMathMLSelectedFrame::ChildListChanged() both call GetSelectedFrame(), even
> though one chains up to the other.

So remove the GetSelectedFrame() call from mactionFrame.

r+ with that change and this patch folded together with attachment 677520 [details] [diff] [review] into a single patch.  (See hg help qfold for one way to do that.)

There is a small merge conflict to fix up as nsIDOMEventTarget.h is no longer
included.

Comment 22

[Frédéric Wang (:fredw)]

Attachment: Patch V4

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 747534 [details] [diff] [review]
Patch V4

>Improve how <semantics> determine the visible child - part 1. b=745131, r=karlt.
>* * *
>Improve how <semantics> determine the visible child - part 2. b=745131, r=karlt.

Just use qref to rewrite the commit message as one line.

Comment 24

[Frédéric Wang (:fredw)]

Attachment: Patch V4

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9354202c68f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e674ea03480b

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/mozilla-central/rev/9354202c68f0
https://hg.mozilla.org/mozilla-central/rev/e674ea03480b

Comment 27

[Frédéric Wang (:fredw)]

I updated the status and testsuite pages.

The selection of the visible semantics child is given by the following algorithm:

http://mxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLsemanticsFrame.cpp#26

Comment 28

[Florian Scholz (Open Web Docs)]

Updated:
https://developer.mozilla.org/en-US/docs/Web/MathML/Element/semantics

Added a note to:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/23#MathML

Editorial / technical review is always welcome :)