Closed Bug 745131 (semantics2) Opened 8 years ago Closed 6 years ago

Improve how <semantics> determine the visible child

Categories

(Core :: MathML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: fredw, Assigned: fredw)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 5 obsolete files)

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: nobody → i
Status: NEW → ASSIGNED
Assignee: i → nobody
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
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
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: nobody → fred.wang
Keywords: helpwanted
Whiteboard: [mentor=fredw][lang=c++]
Attached file testcase
Attached patch Patch V1 (obsolete) — Splinter Review
So here is a first patch that tries to reproduce the behavior discussed elsewhere. Still a WIP.
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?
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
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
Attached patch Patch V2 (obsolete) — Splinter Review
Attachment #677519 - Flags: review?(karlt)
Attached patch Reftests V1Splinter Review
Attachment #677520 - Flags: review?(karlt)
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
(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.
Karl: any update on the review?
Flags: needinfo?(karlt)
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-
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
Whiteboard: [mentor=fredw][lang=c++]
Status: ASSIGNED → NEW
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Whiteboard: [mentor=fredw][lang=c++]
Attached patch Patch V3 - part 1 (obsolete) — Splinter Review
Attachment #677519 - Attachment is obsolete: true
Attachment #736783 - Flags: review?(karlt)
Attached patch Patch V3 - part 2 (obsolete) — Splinter Review
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+
Attached patch Patch V4 (obsolete) — Splinter Review
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+
Attached patch Patch V4Splinter Review
Attachment #747534 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9354202c68f0
https://hg.mozilla.org/mozilla-central/rev/e674ea03480b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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
You need to log in before you can comment on or make changes to this bug.