The default bug view has changed. See this FAQ.
Bug 745131 (semantics2)

Improve how <semantics> determine the visible child

RESOLVED FIXED in mozilla23

Status

()

Core
MathML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

Trunk
mozilla23
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → i
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Assignee: i → nobody
(Assignee)

Comment 1

5 years ago
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
(Assignee)

Updated

5 years ago
Summary: <semantics> should try to display its first child annotated as presentation MathML → <semantics> should try to display its first child annotated of known encoding
(Assignee)

Updated

5 years ago
Summary: <semantics> should try to display its first child annotated of known encoding → <semantics> should try to display its first annotation child of known encoding
(Assignee)

Updated

5 years ago
Assignee: nobody → fred.wang
Keywords: helpwanted
Whiteboard: [mentor=fredw][lang=c++]
(Assignee)

Comment 2

5 years ago
Created attachment 676694 [details]
testcase
(Assignee)

Comment 3

5 years ago
Created attachment 676696 [details] [diff] [review]
Patch V1

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

Comment 4

5 years ago
Bill: could would please include this patch in your builds, so that people could give their advice on the patch behavior.
(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?
(Assignee)

Comment 6

5 years ago
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/fred.wang@free.fr-beae9064e260
Keywords: dev-doc-needed
Summary: <semantics> should try to display its first annotation child of known encoding → Improve how <semantics> determine the visible child
(Assignee)

Comment 7

5 years ago
Created attachment 677379 [details]
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>.
Attachment #676696 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 677519 [details] [diff] [review]
Patch V2
Attachment #677519 - Flags: review?(karlt)
(Assignee)

Comment 9

5 years ago
Created attachment 677520 [details] [diff] [review]
Reftests V1
Attachment #677520 - Flags: review?(karlt)
(Assignee)

Comment 10

5 years ago
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/fred.wang@free.fr-beae9064e260
(Assignee)

Comment 11

5 years ago
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
Blocks: 557086
(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.
(Assignee)

Comment 13

4 years ago
Karl: any update on the review?
Flags: needinfo?(karlt)
(Assignee)

Updated

4 years ago
Flags: needinfo?(karlt)
(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 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.
Attachment #677520 - Flags: review?(karlt) → review+
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.
Attachment #677519 - Flags: review?(karlt) → review-
(Assignee)

Comment 17

4 years ago
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.
Assignee: fred.wang → nobody
Keywords: helpwanted, student-project
Whiteboard: [mentor=fredw][lang=c++]
(Assignee)

Updated

4 years ago
Status: ASSIGNED → NEW
(Assignee)

Updated

4 years ago
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Keywords: helpwanted, student-project
Whiteboard: [mentor=fredw][lang=c++]
(Assignee)

Comment 18

4 years ago
Created attachment 736783 [details] [diff] [review]
Patch V3 - part 1
Attachment #677519 - Attachment is obsolete: true
Attachment #736783 - Flags: review?(karlt)
(Assignee)

Comment 19

4 years ago
Created attachment 736784 [details] [diff] [review]
Patch V3 - part 2
Attachment #736784 - Flags: review?(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.
Attachment #736783 - Flags: review?(karlt)
Attachment #736783 - Flags: review-
Attachment #736783 - Flags: feedback+
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.
Attachment #736784 - Flags: review?(karlt) → review+
(Assignee)

Comment 22

4 years ago
Created attachment 747534 [details] [diff] [review]
Patch V4
Attachment #736783 - Attachment is obsolete: true
Attachment #736784 - Attachment is obsolete: true
Attachment #747534 - Flags: review?(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.
Attachment #747534 - Flags: review?(karlt) → review+
(Assignee)

Comment 24

4 years ago
Created attachment 747872 [details] [diff] [review]
Patch V4
Attachment #747534 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9354202c68f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e674ea03480b
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9354202c68f0
https://hg.mozilla.org/mozilla-central/rev/e674ea03480b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Assignee)

Comment 27

4 years ago
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
Depends on: 876701
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 :)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.