Closed Bug 122524 Opened 23 years ago Closed 22 years ago

Add "View Selection/MathML Source" to the selection context menu

Categories

(SeaMonkey :: UI Design, enhancement)

Product:
SeaMonkey
Component:
UI Design
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.1alpha

People

(Reporter: rbs, Assigned: rbs)

References

Details

Attachments

(2 files, 13 obsolete files)

screenshot -- here is how the integration looks like
19.07 KB, image/png
Details
updated patch to stay in sync after fixing cvs conflicts from recent changes in the trunk
83.42 KB, patch
alecf
: superreview+
Details | Diff | Splinter Review 
doron, do you mind helping out with this? (MathML related - but the GUI side).

I am reproducing what I said in bug 49721 comment 14 regarding the partial 
viewsource of MathML:

<snip>
Check out what I did for MathML as a matter of necessity:
screenshot - Partial viewsource of MathML (WYSIWYG)
http://bugzilla.mozilla.org/attachment.cgi?id=66993&action=view

Since MathML documents tend to be large due to the fact that the number of 
MathML tags exceeds the actual content by far, having the ability to locate the 
partial source of a MathML fragment is useful for those who want to inspect,
learn, or... curl the markup. Also, it helps to see the Unicode points or entity 
names in case of missing glyphs -- something frequent when users lack MathML 
fonts. See http://groups.google.com/groups?hl=en&th=719389d8ff974cb9&rnum=1 and
grab/build a recent MathML-enabled binary to see it live on the MathML demos at:
http://www.mozilla.org/projects/mathml/update.html

(And BTW, in addition to the initial intention of saving from the hassle of 
figuring out what a fragment looks like, getting the partial WYSIWYG is much 
faster that the not-so-fast viewsource.)
</snip>

If you can set a build with MathML-enabled (which is pretty straightforward, see 
the MathML project page), then you can experiment this for yourself. The way 
this works now requires attaching the scripts to all MathML pages and setting an 
onload handler on these pages. But what I would like now is to hook this to the 
context menu instead. Thus it will allow people to right-click on a MathML spot, 
see an item "View MathML WYSIWYG Source" added above "View Page Source", and if 
they pick this item, a viewsource-like windows (like in the screenshot) pops up.

As for the implementation, what may seem natural would be to have a 
'this.onMathML = false/true' in nsContextMenu.js and do the similar gymnastics 
that the other things are doing, namely:

1)
               // MathML? (XML is case-sensitive)
               if (elem.localName === "math" &&
                   elem.namespaceURI === "http://www.w3.org/1998/Math/MathML") {
                   this.onMathML = true;
               }

2)
    initViewItems : function () {
+      // View MathML WYSIWYG Source depends on whether we are on MathML.
+      this.showItem( "context-viewmathmlwysiwyg", this.onMathML );
    [...]  

3)
<!ENTITY viewMathMLWYSIWYGSourceCmd.label     "View MathML WYSIWYG Source">
<!ENTITY viewMathMLWYSIWYGSourceCmd.accesskey "M">

4) have viewMathMLwysiyg.xul (similar to viewSource.xul but with a smaller 
default window size) and viewMathMLwysiyg.js that collates the scripts that I 
already wrote.

[5) In the future, whoever fixes bug 49721 could s/MathML/Partial/g and iterate 
from there to get a general partial WYSIWYG source rooted at desired nodes (the 
MathML one being rooted at the top <math>...</math> level, and rooting at 
<html>...</html> would then amounts to the total WYSIWYG).]

Interested to pick up the torch for the less ambitious MathML context for now?
(BTW, my interest in viewsource has been motivated by the fact that MathML 
source is highly verbose and hard to decipher. That's why niceties in viewsource 
do help here.)
Severity: normal → enhancement
Depends on: 75338
QA Contact: sairuh → pmac
doronr, any chance to get some hot action here?
sorry about that, though I had added a comment before

I'm gone from my tree till next week, so I'll take a look at this then. Also
need to get mathml into my build.

It shouldn't be hard to add it. The only issue is to hide it if svg is not
installed, which I am not sure is possible from js.
MathML is already part of the default builds and will ship in m0.9.9 & onwards.
As for detecting whether it is disabled, let's assume that this fix is done
using a controlling boolean that might be set properly if bug 109825 is fixed.
The feature to quickly view the MathML code is nice, but it should be noted on the
test page that JavaScript has to be enabled to make it work.
http://www.mozilla.org/projects/mathml/demo/basics.xhtml
This test page is directly linked from the 0.9.9 release notes, so many people
might want to test it.
>It shouldn't be hard to add it.

doronr, any good news to share with us? Looks like this will be a worthwhile
addition for m1.0.
  // MathML? (XML is case-sensitive)
               if (elem.localName === "math" &&
                   elem.namespaceURI === "http://www.w3.org/1998/Math/MathML") {
                   this.onMathML = true;
               }

localname seems to not be working (returns null or something like mroot for a
square root mathml symbol)  namespaceURI works however.  Should I just use that?
(I have the context menu item done).

Also, 
Ah, OK. I see. <math> is not a leaf tag. It is instead the top-math element:
<math>
  <mroot>...</mrow>
  ...etc..
</math>

So yes, just test the namespaceURI to catch any MathML tag (which is what is
intended). 

For the null localName, do you get this on text nodes? If so that would mean 
that you may have to test the namespace its parent node for reliablity.
My original message got cutoff for some reason.

Will a non-mathml enabled build return for namespaceURI the MathML namespace? If
not, then it would solve the check if mathml is enabled
Yes, the namespace will be returned. But I am not too worried about it because
if it becomes an issue, then as a work-around (if bug 109825 isn't fixed), there
could be a pref that can be queried, e.g., browser.mathml.enabled.
Yes, it was null on text.

For the napespace getting, it also returns null for text nodes, so I will have
to check its parent's namespace.

<mroot><mn>3</mn><mrow><mi>x</mi><mo>-</mo><mn>3</mn></mrow></mroot>


text nodes return null for namespaceURI. Is this as intended?
Status: NEW → ASSIGNED
From
http://www.w3.org/TR/2000/REC-DOM-Level-2-Core-20001113/core.html#ID-1950641247:

namespaceURI of type DOMString, readonly, introduced in DOM Level 2
    For nodes of any type other than ELEMENT_NODE and ATTRIBUTE_NODE and nodes
    created with a DOM Level 1 method, such as createElement from the Document
    interface, this is always null.

So yes, this is as intended.  :)
>For the napespace getting, it also returns null for text nodes, so I will have
>to check its parent's namespace.

yes, just do:

  // MathML?
  const mathmlNS = "http://www.w3.org/1998/Math/MathML";
  if ((elem.nodeType == Node.TEXT_NODE &&
       elem.parentNode.namespaceURI == mathmlNS) || 
      (elem.namespaceURI == mathmlNS)) {
    this.onMathML = true;
  }

If you've got a patch now, do well to attach for r/sr/a to allow some time
for the fixups.
Do you want any menus or context menus?
Attached patch patch (obsolete) — — Splinter Review 
With a dummy js file
No longer depends on: 75338
Patch 76050 looks good.
Nit: 
+  if (focusedWindow)
+    var docCharset = "charset=" + focusedWindow.document.characterSet;
+ 
might be written as (JS is forgiving...):
+  var docCharset = (focusedWindow)
         ? "charset=" + focusedWindow.document.characterSet
         : nsnull;

> Do you want any menus or context menus?

Yeah, that might be nice. Actually, the default options that the normal
viewsource look sastisfactory. Except that "File->Edit Page" can be grayed out
since it won't work.
+  if (focusedWindow)
+    var docCharset = "charset=" + focusedWindow.document.characterSet;
+ 
might be written as (JS is forgiving...):
+  var docCharset = (focusedWindow)
         ? "charset=" + focusedWindow.document.characterSet
         : nsnull;

That is how the original viewsource.js was written, I just copied it :)


>> Do you want any menus or context menus?

>Yeah, that might be nice. Actually, the default options that the normal
>viewsource look sastisfactory. Except that "File->Edit Page" can be grayed out
>since it won't work.

OK, will overlay them.  Do you have your code somewhere so I can add it?
Depends on: 75338
http://www.mozilla.org/html/projects/mathml/demo/entity.js
http://www.mozilla.org/html/projects/mathml/demo/viewmath.js

which you can just concat as a single file in your viewMathMLSource.js
I just updated viewmath.js to sync it with the recent changes that bz did in the
code tree.
         : nsnull;
JS is forgiving, but it has null, not nsnull ;-)

gila and www have similar paths, but there's no html/ in www...
http://www.mozilla.org/projects/mathml/demo/entity.js
http://www.mozilla.org/projects/mathml/demo/viewmath.js
Why does this need implementing separately from normal selection source? What 
if I select a span of text which starts a few characters before the end of a 
MathML chunk, and ends a couple of characters after HTML resumes? I'd expect 
the same thing to happen as if the whole selection was HTML: the source for the 
selected content being pre-selected in the view source window. Then, if I clear 
the selection and do the opposite -- select some source (partly MathML, partly 
HTML) in the source window, and choose `View' > `Page' -- the same content is 
selected when the page loads in the browser window.
re: comment #19... thanks timeless! I had too many things flying in my head...

re: comment #20: see point 5 in my opening comment. Iterations can always comme
later. If someone is interested in re-writing viewMathMLSource.js to a more
general viewSelectedSource.js, then of course, it will be of wider benefit. But
nobody has been thrilled about bug 49721 and bug 28809, and so there hasn't been
any tractions over there (note their low bug numbers).

As implemented presently, the segment to highlight in the partial source is
determined by the mouse pointer. The pointer can only be in one place at any one
time: inside or outside the "root" <math>...</math>. Moreover, the source is
displayed as a nicely reformatted WYSIWYG source, directly via the DOM.

A <math> fragment, like an <img>, is highly distinguishable from other pieces on
the page, and so it makes sense to me to fix this bug rather than waiting on
other uncertain bugs. The functionality has been receiving very positive
feedback re: math fragments in particular (I listed some reasons why this might
be so in the opening comments). If someone iterates and folds this functionality
in bug 28809 in due time, then of course it will be of wider applicability.
-> nominating for mozilla1.0 mindful of the fact that the patch is nearly ready.
Keywords: mozilla1.0
Target Milestone: --- → mozilla1.0
will get this done tomorrow
After fighting overlays to play nice, I have 2 questions:

1. I need to change the code to not open a window.  

As far as I understand, getOuterMarkup(m,0) is what will return me the html code
for the markup and m is the <mathml> node.

I am not sure how to make it work with viewsource. I can get a <browser> with
the html in it, but that will be formatted by the viewsource code and I think
that does not have a DOM tree. Also, I will need to circumvent the syntax
colouring somehow for this, as then the DOM tree will be messed.

2) The js is not triple licenced, and can't go into the tree.  We need
confirmation from anyone who added code (rbs and  Steve Swanson
<steve.swanson@mackichan.com>) to agree to this.
> I am not sure how to make it work with viewsource.

I'm not sure what the rest of your paragraph means, but what it _sounds_ like 
you want to do is to take the <math> node and all the stuff under it, serialize 
it, create a data: url of type text/xml, and view-source that.  Or am I missing 
something?
Attached file viewsource using the data: protocol (obsolete) — 

    
    

    
  
>After fighting overlays to play nice, I have 2 questions:

Thanks for taking the time to fix this bug.

>1. I need to change the code to not open a window.  
>
>As far as I understand, getOuterMarkup(m,0) is what will return me the html
code
>for the markup and m is the <mathml> node.
>
>I am not sure how to make it work with viewsource. I can get a <browser> with
>the html in it, but that will be formatted by the viewsource code and I think
>that does not have a DOM tree. Also, I will need to circumvent the syntax
>colouring somehow for this, as then the DOM tree will be messed.

See attachment 76788 [details], hope it clarifies your question. What is needed
is a browser window that has a specific content.

if you look at the attachment and do a dump:
      open("data:text/html;charset=US-ASCII," + src, "wysiwyg",
                               "scrollbars=1,resizable=1,width=500,height=200");
+     dump("data:text/html;charset=US-ASCII," + src);

and copy-paste the dump in the location bar, it renders the same way as
viewsource does (I tested). So you need to open a browser window with
the URL as: 
  data:text/html;charset=US-ASCII," + src

Of course "US-ASCII" can be substituted with the actual doc charset.

[And BTW, if you do: viewsource:data:text/html;charset=US-ASCII," + src,
it will actually do a viewsource-of-viewsource...]

Side-note for bz: there is a bug in the data protocol, there is a crash
in nsDataChannel::ParseData if you don't specify the charset. I had a
quick look and spotted at the flaw in that code:
        char *semiColon = PL_strchr(buffer, ';');
        if (semiColon)
            *semiColon = '\0';
[...]
        char *charset = PL_strcasestr(semiColon + 1, "charset=");
-> access violation when semiColon is null.

>2) The js is not triple licenced, and can't go into the tree.  We need
>confirmation from anyone who added code (rbs and  Steve Swanson
><steve.swanson@mackichan.com>) to agree to this.

Feel free to replace with the tri-license, I am cc:ing Steve to the bug for 
confirmation.


    
    

    
  
I'm happy to contribute this code to Mozilla, under whatever license you like. 
Do I need to do anything besides make this remark?

    
    

    
  
> there is a bug in the data protocol, there is a crash
> in nsDataChannel::ParseData

Yeah.  I saw a patch for that problem recently being either checked in or 
reviewed....

    
    

    
  
Just wanted to say that I am leaving on tuesday for the US to start at netscape
evangelism, so will try to get this fixed beforehand.

    
    

    
  
Ok, have this almost working.  I had to drop overlaying the viewsource xul and
simply replicated the exact code, as it was including files twice and causing
lots of js warnings.

All that is left is making the DOM tree highlighted using the js files you
included.  

What I currently do is serialize the <mathml> tree, send it via window.arguments
to viewMathMLSource.xul, where I use a DOMParser() object to recreate the tree.

    
    

    
  
Another problem:

  var parser = new DOMParser();
  var myDOM = parser.parseFromString (window.arguments[2],"text/xml");

where window.arguments is the mathml text from
http://www.w3.org/Math/testsuite/Presentation/GeneralLayout/msqrt-mroot/mrootB1.xhtml

Gives me:

<parsererror xmlns="http://www.mozilla.org/newlayout/xml/parsererror.xml">XML
Parsing Error: not well-formed Location: about:blank Line Number 3, Column 26:
<sourcetext>&lt;mroot&gt;&lt;mn&gt;3&lt;/mn&gt;&lt;mrow
-moz-math-font-size="scriptminsize"&gt;&lt;mi
-moz-math-font-style="italic"&gt;x&lt;/mi&gt;&lt;mo&gt;-&lt;/mo&gt;&lt;mn&gt;3&lt;/mn&gt;&lt;/mrow&gt;&lt;/mroot&gt;
-------------------------^</sourcetext></parsererror>

Any ideas?

    
    

    
  


  
>What I currently do is serialize the <mathml> tree, send it via
>window.arguments to viewMathMLSource.xul, where I use a DOMParser() object to
>recreate the tree.
>
> var parser = new DOMParser();
> var myDOM = parser.parseFromString (window.arguments[2],"text/xml");

No need for these. It will make this depends on xmlextras (i.e, it will break
if /extensions/ are not built); it won't convert the entities back to their
MathML names; and I am not sure whether it will provide a nice indentation of
the resulting markup.

I have attached a file which shows that you can just call serializeMathML(node)
to get the stringified version of the portion of the DOM of interest. Once you
have this, opening a window with the URL set via the 'data:' protocol as I
described earlier should work.

    
    

    
  
c.f. what is being done for the metada. The target node is passed as arugment, 
    showMetadata : function () {
        window.openDialog(  "chrome://navigator/content/metadata.xul",
                            "_blank",
                            "scrollbars,resizable,chrome,dialog=no",
                            this.target);
    },

and the genuflexions for the rendering happen in medata.js.

It will actually be more effective to proceed for this bug in a similar way, 
i.e., pass the node as argument, do the serialization in viewMathMLSource.js, 
and dump the result in the output window. Hence the 'data:' protocol is not even 
needed. And the fact that the viewMathMLSource.js script will only be executed 
if a MathML node is encountered will ensure no perf regressions.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
What I have. It currently renders empty, though the correct markup is created.

        Attachment #76050 -
        Attachment is obsolete: true

    
    

    
  
+    <browser id="content" type="content-primary" name="content"
src="about:blank" flex="1"
+             context="viewSourceContextMenu"/>

So, the aim is to replace the blank content with the output you got in:
+       onload="onLoadViewSource();"

That's why the 'data:' protocol won't be suitable.

It instead needs: <getbrowser>._content.document.write(serializerOutput)


    
    

    
  
actually, since it is <browser/>, it is a bit more complex. Got this working,
patch coming

    
    

    
  

      
      
      
      

        Attached patch
          
          
        working patch (obsolete)
        — 
          — Splinter Review
      

    


  
Working version

        Attachment #76960 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 77039 [details] [diff] [review]
working patch

>Index: xpfe/browser/resources/content/navigator.js

Why is the code in navigator.js? Might be safer to put it in
nsContextMenu.js (in a similar way to what metadata is doing)
to avoid any impact on page load performance.

+  //walk up the tree to the <mathml> tag
spelling: <math> tag

+  while (node && node.nodeName.indexOf("math") == -1)
+    node = node.parentNode;

Add null check:
   if (!node)
     return'

(This way, the code is safe against the case where there is
a bad markup that is not surrounded in <math>...</math>)

+  window.openDialog("chrome://navigator/content/viewMathMLSource.xul",
+	      "_blank",
+	      "scrollbars,resizable,chrome,dialog=no",
+	      getWebNavigation().currentURI.spec, docCharset,node);

nit: spacing in "docCharset,node"

    
    

    
  
I won't have a tree till middle if not end of next week...

    
    

    
  


  
Since time is running out, I have applied your patch iterated over it to
tidy/consolidate the JS, and switch to an overlay shared both the normal view
source and the partial viewsource. A couple of glitches remain ATM: 1) there is
an extra separator after the last item of the 'Help' menu, 2) the menu commands
don't work (in my debug build, an assertion is firing saying that the
<menuitem>s are attached to commands that doesn't exist -- but since I am off,
I am attaching what I have so far in case someone wants to iterate too over the
weekend. All the functionalities are in place (both normal viewource and
partial viewsource show up), escept for the two things that I mentioned.

        Attachment #76788 -
        Attachment is obsolete: true

        Attachment #76948 -
        Attachment is obsolete: true

        Attachment #77039 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 77796 [details] [diff] [review]
iteration to tidy things and switch to an overalay

Typos: these sould be sraight |return;|

+  if (gEntityTable)
+    return gEntityTable;
+
+  gEntityTable = new Array(1480);
+  if (!gEntityTable)
+    return null;

    
    

    
  

      
      
      
      

        Attached patch
          
          
        final patch, ready for r/sr/a (obsolete)
        — 
          — Splinter Review
      

    


  
This version of the pacth has no known issues. It consolidates the use of an
overlay, and resolves the issues I mentioned earlier. The behavior of the 
previous viewsource is back, and the setup is extended to handle the partial
MathML viewsource, thereby providing building blocks for someone to substitute
these entry points with a general "View selection" or something like that
later.

The problem with the commands that were not working was because the XUL insists

on using '<commandset id="commands"/>' rather than 
'<commandset id="VIEWSOURCE-commands"/>'.

The extra separator was because of multiple inclusion of overlays that confuses
things. I changed to only include navigatorOverlay.xul, and the extra separator
went away. Overall, finding which IDs to retain and those that _must_ change
was a fight and I can see how it was a tough time for you too doron :-)

Other things that I noted and that might interest others in the future in other
similar situations: one must set contenttitlesetting="true", *and* set the
title empty, title="", otherwise the title set in the HTML source doesn't take
effect. (This is not the desired intention here, since we want the locazilized
title to override). I am pointing this out because dealing with this title
business always seems to be the source of much woes (witness the past
experience we had with the normal viewsource). It took me some voodoo to figure
out how to make the title from the serialized HTML source show up on the window
-- although as I said, it isn't the intention here. So I am jotting the tip
down.

        Attachment #77796 -
        Attachment is obsolete: true

    
  
Summary: Add "View MathML WYSIWYG Source" to the context menu → [fix, awaiting r/sr/a] Add "View MathML WYSIWYG Source" to the context menu
Whiteboard: awaiting r/sr/a

    
    

    
  
...I forgot to disable the 'Edit Page' menu. Now corrected in my tree, I have 
added an id="editpage" on the relevant menu item and added a statement in 
onLoadViewMathMLSource() to just do:

  // disable the editpage menu item since the editor doesn't work for MathML
  document.getElementById('editpage').setAttribute("disabled", "true");
Summary: [fix, awaiting r/sr/a] Add "View MathML WYSIWYG Source" to the context menu → Add "View MathML WYSIWYG Source" to the context menu
Whiteboard: awaiting r/sr/a

    
  
Summary: Add "View MathML WYSIWYG Source" to the context menu → [fix, awaiting r/sr/a] Add "View MathML WYSIWYG Source" to the context menu
Whiteboard: have fix, awaiting r/sr/a

    
    

    
  
mpt, do you have a better suggestion than "...MathML WYSIWYG Source"? These are 
the only customizable strings exposed, and can be set as usual via .dtd files. 
So there is no really code-level changes to replace them if there is a beter 
suggestion. The thing is that the popup window is rendered from the DOM rather 
that the plain source. Indeed, the actual fragment in the source might be messy 
(e.g., the whole <math>...</math> might be in a single line with no spacing 
between tags at all, making it even harder to read without such a "View MathML 
WYSIWYG Source"...) I didn't sign up on "View MathML Source" in order to hint at 
the difference, but maybe so much precision isn't needed here.

    
    

    
  
I'd rather see "View MathML Source". I've never understood what the WYSIWYG 
meant

Since the inclusion on the popup is context-sensitive, I think the actual 
behavior is clear.

True, it isn't really the source you're seeing, but a formatted version of 
what's in the DOM.  If "Source" interacts too much with the main "View Source" 
entry, how about "View MathML Markup"?

    
    

    
  
Comment on attachment 77957 [details] [diff] [review]
final patch, ready for r/sr/a

>+  while (topNode && topNode.nodeName.indexOf("math") == -1)

Shouldn't this be something like |topNode.localName == "math"| ?
I'm not convinced of this "indexOf" usage....

>+    // to avoid the wide gap problem, '\n' is not emitted on the first
>+    // line and the lines before & after the <pre id="target">...</pre>
>+    newline = '';
>+    if (gLineCount > 0) {
>+      newline = '\n';
>+    }
>+    if (gLineCount == gStartTargetLine ||
>+        gLineCount == gEndTargetLine) {
>+      newline = '';
>+    }

Could we have a single logical test that sets |newline|?

>+      if (text[i] == '<')
>+        str += '&amp;<span class="entity">lt;</span>';
>+      else if (text[i] == '>')
>+        str += '&amp;<span class="entity">gt;</span>';
>+      else
>+        str += text[i];

What about escaping '&'?

>Index: xpfe/browser/resources/content/viewMathMLSource.xul

This needs to be tri-licensed.

>Index: xpfe/browser/resources/content/viewSourceOverlay.xul

So does this.

>+<!ENTITY mainWindow.titlemathmlsource "MathML WYSIWYG Source">
>+

Why not just "MathML Source"?  Or maybe "Generated MathML Source"?

The rest looks fairly good.

        Attachment #77957 -
        Flags: needs-work+

    
    

    
  
> Shouldn't this be something like |topNode.localName == "math"| ?
> I'm not convinced of this "indexOf" usage....

In the XML/XHTML world, the mere eqaulity test breaks down when there is a
namespace prefix, e.g., "<m:math>".

> Could we have a single logical test that sets |newline|?
Every time when I look at that code, I wonder why I am not doing that :-)

I have a new patch that also fixes bug 49721 for UI people. I will attach an
updated patch with bz's suggestions later. BTW, does "View Selection Source"
catch you on (as the label to use for bug 49721)?

    
    

    
  
> In the XML/XHTML world, the mere eqaulity test breaks down when there is a
> namespace prefix, e.g., "<m:math>".

Except I suggested testing .localName, not .nodeName.  .localName has the
namespace prefix stripped.

"View Selection Source" sounds pretty good.  :)

    
    

    
  


  
bz, here you have it...

For bug 49721/bug 28809, here is a screenshot of what the patch does:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=79770

        Attachment #77957 -
        Attachment is obsolete: true

    
  
Summary: [fix, awaiting r/sr/a] Add "View MathML WYSIWYG Source" to the context menu → [fix, awaiting r/sr/a] Add "View MathML Source" to the context menu

    
    

    
  
> +  // let the ancestor be an element
> +  if (ancestorContainer.nodeType == Node.TEXT_NODE ||
> +      ancestorContainer.nodeType == Node.CDATA_SECTION_NODE)
> +    ancestorContainer = ancestorContainer.parentNode;

Wouldn't checking for != Node.ELEMENT_NODE be better?

What happens if the SELECTION_END/SELECTION_START marks are already present in
the text of the page?  Wouldn't it be better to use more unlikely markers?

> +    if (n == ancestor || !p)
> +      break;

You don't need the !p check here, since the while loop checks it.

Is it correct to restore the same values into findService and findInst?

> +  + 'function scrollToTarget() {'
> +  + '  scrollTo(0, document.getElementById("target").offsetTop);'
> +  + '}'

Hmm... Is it better to do that or to use
|document.getElementById("target").scrollIntoView(true);|

Don't change the "Original Developer" on viewSource.xul.

> +        // "View Selection Source" and "View MathML Source" are mutually
> +        // exclusive, with the precedence given to the selection when there

That comment seems to die in mid-sentence... you wanted "when there is one." ?

Please have the actual menuitems pass in something saying what source of partial
source you're viewing.  This way if we change the selection stuff so that the
selection context menu only comes up when clicking on _selected_ source you
won't be getting things like a selection outside MathML, right-click on MathML,
view partial source viewing the selection source.



ccing marlon so that he has a chance to comment on adding the new item to the
"on selected text" menu.






    
    

    
  

      
      
      
      

        Attached patch
          
          
        iteration after bz's feedback  (obsolete)
        — 
          — Splinter Review
      

    


  
> Wouldn't checking for != Node.ELEMENT_NODE be better?

I am playing safe here, plus it is a selection -- so it should
usually be text/cdata, or an ancestor of it. (in case of unexpected
troubles, the clause will simply keep the context tight around the selection.)

> What happens if the SELECTION_END/SELECTION_START marks are already present
in
> the text of the page?  Wouldn't it be better to use more unlikely markers?

It will mess the selection... (not fatal -- but users are unlikely to ever get
there). Actually, there are special Unicode control sequences to delimit a
selection (as in a man page):

U+0086 START OF SELECTED AREA
U+0087 END OF SELECTED AREA

However, the font back-end doesn't special-case such sequences and they will
show up as weird glyphs anyway if they were to be displayed. But I have now
changed the markers to use 5 &zwp; characters (ZERO WIDTH SPACE). Fingers
crossed that such a sequence won't be common... (or could be 10 &zwsp; ...)

> You don't need the !p check here, since the while loop checks it.

I have switched to a |do ... while| loop. It is much neater now.

> Is it correct to restore the same values into findService and findInst?

Apparently yes. That's what I ended up doing upon testing (so that the internal
finds that I am doing don't leak out to a user who later pops the find dialog).
The documentation in nsIFindService says:

     * The sole purpose of the Find service is to store globally the
     * last used Find settings
       ^^^^
> Hmm... Is it better to do that or to use
> |document.getElementById("target").scrollIntoView(true);|

Nice.

> Don't change the "Original Developer" on viewSource.xul.

Beware of copy-paste...

> That comment seems to die in mid-sentence... you wanted "when there is one."
?

Added.

> Please have the actual menuitems pass in something saying what source of
> partial source you're viewing.

Done. Indeed it is better to pass the context as argument.

        Attachment #79774 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        final patch (obsolete)
        — 
          — Splinter Review
      

    


  
This patch includes some tydings:
- some tabs crept in the earlier patch, I converted them to whitespace;
- renamed viewPartialSourceForMathML() to viewPartialSourceForFragment().
Indeed, the only thing that is really MathML-specific in this whole business is
the test tagName=='math'. (With additional clauses, e.g., tagName=='svg', it is
already possible to peek at other fragments. This may be very useful in such
contexts.)

        Attachment #79934 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        take this one instead (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #80016 -
        Attachment is obsolete: true

    
    

    
  
-> off-loading from doron and re-assigning to myself following his trip at the 
Netscape campus in comment #30 and the new developments since comment #40.
We thought this thing could be wrapped up before the branch & RC1 & etc :-(
Assignee: doron → rbs
Status: ASSIGNED → NEW

    
    

    
  
Comment on attachment 80038 [details] [diff] [review]
take this one instead

r=bzbarsky (though I did not check gEntityTable for correctness of all the
entities; I trust you used some semi-automated method to generate those
entries....)

        Attachment #80038 -
        Flags: review+

    
    

    
  
The list was generated from the XHTML + MathML DTDs:
http://lxr.mozilla.org/seamonkey/source/layout/mathml/content/src/mathml.dtd

I have added a comment to that effect.
Status: NEW → ASSIGNED
Summary: [fix, awaiting r/sr/a] Add "View MathML Source" to the context menu → [fix, have r=, awaiting sr=/a=] Add "View MathML Source" to the context menu
Whiteboard: have fix, awaiting r/sr/a → have fix, have r=bzbarsky, awaiting sr/a

    
    

    
  
Cc:ing alecf for sr=

    
    

    
  
ack! not another item added to the context menu. I want to see some UI buy-in
before I even look at this patch.

    
    

    
  
alecf, did you take into account the conditions under which the item shows up?
Pretty specific and rare/limited actually.

    
    

    
  
it doesn't matter what I take into account - I'm not a UI owner and I don't see
any input from any UI owners here.. once I see some kind of acceptance, I'll
super review it.

    
    

    
  
I would think mpt is happy with how things are coming along since it implements 
what he wrote in bug 49721 comment 17, and blaker happens to be the one who 
filed bug 49721. mpt/blaker if you disagree with something here do well to tell.

    
    

    
  
ccing marlon as well (as a note, the selected text context menu is one in which
Marlon _was_ considering accepting features).

    
    

    
  
This option is proving really valuable once there, especially re: dev/debug/QA: 
"what is going on here?", "how do they do that?", and people best learn things 
from how others are doing.

View-source has remained unparallel for quick learning, and I am hoping that the 
magic of view-source would help other specialized markups such as MathML too, as 
in the quote (^H s are mine):

"And the browser "view source" command made HTML^H^H^H^HMathML a common
vernacular." -- http://www.oreilly.com/catalog/opensources/book/netrev.html


    
    

    
  
alecf, what do you think of the patch? (knowing that UI owners are informed, 
have requested things along these lines in the past, and are not registering 
complains now w.r.t. the patch).

    
    

    
  
re-summarizing what the patch does:

The aim of the patch is to allow someone to select a piece of content in
the browser window, and to re-map the selection in the view-source markup
when doing view-source (c.f. screenshot below). This could for example allow
debugging/capturing live things generated via JavaScript, or help QA to
quickly extract simplified testcases from complex web pages. It can also help
to capture XSLT generated fragments since XSLT does a DOM-to-DOM transformation
and the classical view-source gives the original static document (i.e. with
the initial XML processing-instruction to invoke XSLT, etc), whereas with
this option now available, one can peek at the actual generated output
without much fuss. The patch also caters for fragments such as MathML by 
rooting the context (in the MathML case) at <math>...</math> -- which is
the top-level container of all MathML markups.

...of note: the patch is verbose due the rather long list of entities
that it contains (from XHTML 1.1 + MathML 2.0).

patch: http://bugzilla.mozilla.org/attachment.cgi?id=80038&action=view
screenshot: http://bugzilla.mozilla.org/attachment.cgi?id=79770&action=view

Related: bug 49721 & bug 28809

    
    

    
  
let me re-summarize: "once I see some kind of acceptance" 

lack of objections is not acceptance. I've been bitten way to many times by this
in the past, and the context menus are particularly sensitive areas for the UI
people. Honestly, it looks fine to me, but I will not put my sr= in here until I
know that someone with some authority over the UI is in agreement.

    
    

    
  
Alec, whom do you consider as having such authority (as in, whom should rbs mail
and pray that the bother to respond)?  I've had a very hard time figuring that
out (and http://mozilla.org/owners.html is not helpful).

    
    

    
  
UI people will ignore this while it contains the word "MathML" in the summary.
Changing it to be more accurate. (I hope.)


As to my personal opinion: I don't personally think we should have a specialised
source viewer in Mozilla; I think we should just use the platform's $EDITOR.
e.g. on my machine view source should launch Emacs. Now View Selection Source in
that world is still a great feature IMHO; it should work like the equivalent
feature in IE when you have the IE powertools installed (but I think it is a
valid feature to include even in a non-powertool-enabled Mozilla build). That
is, it should launch the $EDITOR with only the source for the selection. On the
long run, we could even have it pass the start and end offsets of the selection
to the editor, along with the complete source -- used e.g. with Emacs' ability
to run arbitrary Lisp code, this could have exactly the same effect as now.
Summary: [fix, have r=, awaiting sr=/a=] Add "View MathML Source" to the context menu → [fix, have r=, awaiting sr=/a=] Add "View Selection Source" to the selection context menu

    
    

    
  
Boris Zbarsky wrote:
> I've had a very hard time figuring 
> that out (and http://mozilla.org/owners.html is not helpful).

http://bugzilla.mozilla.org/survey.cgi?action=list should be up-to-date... :)

    
    

    
  
>I don't personally think we should have a specialised source viewer in Mozilla

Yeah, I came across such bugs (view-source in $EDITOR, or even in a tab). But 
let's keep this particular bug in focus, in the light of the current context.

    
    

    
  
the spec is anticipating this feature, please see:

http://mozilla.org/projects/ui/communicator/framework/contextmenus/index.html#Anchor-28258

thanks for working this out, it will be great to see it in action.

    
    

    
  
Intriguing that other UI folks didn't say/know something about this. They obviously
need to be doing their homework properly.

another direct link for those having trouble finding their way from marlon's link:
http://mozilla.org/projects/ui/communicator/framework/contextmenus/index.html#Anchor-52467

alecf, now the ball is back to your court again...

    
    

    
  
Comment on attachment 80038 [details] [diff] [review]
take this one instead

that's the kind of endorsement I need.	Thanks.

I'm reviewing now.

    
    

    
  
Comment on attachment 80038 [details] [diff] [review]
take this one instead

I guess I'm slightly concerned that you're tweaking the actual content, though
I can't really think 
of another option since its going to be hard to find the offset of the data in
.innerHTML. 
I also can't find where you're removing the nodes that you've inserted.

>+  // now extract and display the syntax highlighted source
>+  tmpNode = doc.createElementNS(NS_XHTML, 'div');
>+  tmpNode.appendChild(ancestorContainer);
>+  var loadFlags = Components.interfaces.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE;
>+  getBrowser().webNavigation
>+              .loadURI("view-source:data:text/html;charset=utf-8," + escape(tmpNode.innerHTML),
>+                       loadFlags, null, null, null);

how do you know this is utf8? can't you get the charset of the document? And in
fact, innerHTML will probably give you unicode, 
I wonder how that will get converted in loadURI, since loadURI takes a wstring? 
Can you add a comment here explaining why you can assume this is utf8?

>+
>+  // the load is aynchronous and so we will wait until the viewsoure DOM is done
>+  // before drawing the selection. Using a timer here since |onload| didn't work
>+  // with <browser>, and attempts to use addEventListener("load") failed too.
>+  setTimeout(drawSelection, 100);


eewww... this seems especially bad since you're tweaking content. It sounds
like the "load" event isn't firing
for data URLs. have you looked for a bug on that, or filed one? then you can
put the bug # in the comment.


>+function initEntityTable()

you could save yourself a whole lot of initialization by letting the parser
initialize the array.
var gEntityTable = { "0009" : "Tab",
		     "000A" : "NewLine",
		      etc...};

That said, there isn't ANY way to do this without hardcoding your own entity
table in this JS file?
that just seems broken. There has GOT to be a C++ service around somewhere that
already has these mappings 
(and probably one you stole the table from :))

I suggest figuring out how to make that table scriptable, and accessing that.
This hardcoded table is totally bogus.


The rest of the patch looks fine.

        Attachment #80038 -
        Flags: needs-work+

    
    

    
  


  
>I guess I'm slightly concerned that you're tweaking the actual content, though

>I can't really think of another option since its going to be hard to find the
>offset of the data in .innerHTML.

I couldn't think of another way either.

> I also can't find where you're removing the nodes that you've inserted.

When they go in the innerHTML, they become CTDATA and are removed thus:

+  // delete the special markers now...
+  endContainer.deleteData(endOffset, endLength);
+  startContainer.deleteData(startOffset, startLength);
+  if (startContainer == endContainer)
+    endOffset -= startLength; // has shrunk if on same text node...

> Can you add a comment here explaining why you can assume this is utf8?

Added.

> >+  setTimeout(drawSelection, 100);
>
> eewww... this seems especially bad since you're tweaking content.

It wasn't really bad (I am using "wasn't" because it is now fixed). The markers
remained of invisible 0-length since they were zero-width-space (when
debugging, I had tried a timeout of 5000 to see what was going on in slow
motion. I tried other non-invisible markers and it was fun). Anyway, it is a
non-issue now. The cleaner fix was to add the onload listener on the <vbox>
that contains the <browser>.

> That said, there isn't ANY way to do this without hardcoding your own entity
> table in this JS file?

There is indeed a way and I knew it all along. I am glad that you brought it up
as something to do for the sr=. I have made the necessary changes. They are
simple but large and my patch would have lingered here even longer.

> that just seems broken.

Yep, the attitude problem is broken, see hixie's comment #70 where he had to
remove 'MathML' from the title of this bug in an attempt to move things
forward.
The low-priority that folks attribute to MathML requests can only encourage
minimalist solutions.

> There has GOT to be a C++ service around somewhere that
> already has these mappings 

There is the nsIEntityConverter interface.

> (and probably one you stole the table from :))

I stole it from me -- at least time time...

> I suggest figuring out how to make that table scriptable, and accessing that.


Done.

> The rest of the patch looks fine.

OK.

        Attachment #80038 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 81830 [details] [diff] [review]
updated patch to incorporate the sr's feedback 

ah! I see the content deletion now. 

thanks for cleaning up the entity stuff, I like this solution SO much better :)

However, I talked with jag briefly, and we both agreed that
1) modifying content is bad - it causes reflows, and could conflict with any
running JS that might be tweaking the DOM.
2) using .innerHtml doesn't really buy us anything (I thought it was part of
the DOM standard, but jag pointed out it was just an IE compatiblity hack)

So, I was going to say you should go look at contentAreaDD.js, because I just
remembered that I once wrote code that used the selection API to serialize HTML
from a selection from JS.. but now it looks like someone rewrote this in C++
and its now in 
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsContentAreaDragDrop.
cpp#715

specifically, go check out nsISelectionPrivate::ToStringWithFormat()

(I mention the JS stuff because basically I know that you can write the HTML
serialization code in JS because I've done it - don't be thrown off by this now
being in C++)

so, sorry I can't mark sr= on this yet... since there is a solution where you
can serialize HTML from a selection, I'd really rather see that.

Thanks for cleaning up the entity stuff. If you want to break that into a
seperate patch, I can sr= that seperately.

        Attachment #81830 -
        Flags: needs-work+

    
    

    
  
I think there is a confusion somewhere regarding the tweaks that are done; it is 
a bit more subtle than it seems, let me try to clarify a bit more.

> 1) modifying content is bad - it causes reflows, and could conflict with any
> running JS that might be tweaking the DOM.

There are two things that are happening: from the original window, and from
the view-source window.

A) From the original window (the window where the user does the selection)
  1/ a fragment _bigger_ than just the selection fragment is _cloned_ (the 
rationale for why it has to be bigger is described in bug 49721 comment 26).
  2/ then, the tweaks happen in the _clone_. And since the clone isn't yet (and 
is never) attached to the document, no reflows are triggered whatsoever (i.e., 
there are early returns in the various content observers because the cloned 
fragment remains an orphan for whom |mDocument| is null).

B) From the view-source window (where the selection is re-mapped). Here, the 
live _view-source_ document is modified after it has been constructed. Since 
view-source is internal to Mozilla and doesn't have user-defined scripts, there 
is no way the modifications to remove the markers can conflit with users's 
scripts.

> 2) using .innerHtml doesn't really buy us anything (I thought it was part of
> the DOM standard, but jag pointed out it was just an IE compatiblity hack)

I had an earlier version that used nsISelectionPrivate::ToStringWithFormat() per 
a suggestion from akkana, but I switched back to .innerHTML because 
nsISelectionPrivate::ToStringWithFormat() didn't fit with the fact it is a 
fragment bigger than the selection that is finally processed (c.f. screenshot
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=79770 which shows
the selection in-between other things).

I do not see why |.innerHTML| is really bad just because it orignated from IE. 
In fact, its C++ back-end boils down to the same code as ToStringWithFormat(). 
Moreover, even if just the selection fragment was used, tweaks will still be 
needed because the view-source DOM is an inflated DOM (with markups to do the 
coloring, etc). It would be a nigthmare to try to recover the offsets of the 
selection (with the inflated view-source tags, entity names, etc).

> Thanks for cleaning up the entity stuff. If you want to break that into a
> seperate patch, I can sr= that seperately.

At this stage, I don't have much need for the entity stuff other than in the 
context of this bug. I hope my above explanations have clarified that the entire 
patch is okay.

    
    

    
  
In case I haven't made it clear enough, the re-mapped selection that shows up on 
the inflated view-source output is automatic (i.e., I didn't select it by 
hand, that's the crux of this bug):
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=79770

    
    

    
  
Comment on attachment 81830 [details] [diff] [review]
updated patch to incorporate the sr's feedback 

migrating r=bzbarsky, and re-requesting sr=
IMO, the patch is as good as it can get for what it aims at addressing

        Attachment #81830 -
        Flags: needs-work+ → review+

    
  
Blocks: 105580

    
    

    
  
--> missed m1.0, moving to m1.1alpha.
Keywords: mozilla1.0
Summary: [fix, have r=, awaiting sr=/a=] Add "View Selection Source" to the selection context menu → [fix, have r=, awaiting sr=] Add "View Selection Source" to the selection context menu
Whiteboard: have fix, have r=bzbarsky, awaiting sr/a → have fix, have r=bzbarsky, awaiting sr
Target Milestone: mozilla1.0 → mozilla1.1alpha

    
    

    
  


  

        Attachment #81830 -
        Attachment is obsolete: true

        Attachment #81834 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 82672 [details] [diff] [review]
updated patch to stay in sync after fixing cvs conflicts from recent changes in the trunk

ok, thanks for the explanation. the rest of this patch looks good. sr=alecf

        Attachment #82672 -
        Flags: superreview+

    
    

    
  
Thanks for the sr=alecf. Fix was checked in. I made a typo in the checkin
comment: the bug numbers should have read b=49721,122524 (rather than 1222524).
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED

    
  
Summary: [fix, have r=, awaiting sr=] Add "View Selection Source" to the selection context menu → Add "View Selection/MathML Source" to the selection context menu
Whiteboard: have fix, have r=bzbarsky, awaiting sr

    
    

    
  
Apparently, developers seem to like this feature a lot. I saw the following while
reading a news report at ZDnet http://zdnet.com.com/2100-1104-955520.html

http://forums.com.com/group/zd.News.Talkback/zdnn/tb.tpt/@thread@80928@forward@1@D-,D@ALL/@article@80928?EXP=ALL&VWM=hr&ROS=1&PAGETP=2100&SHOST=zdnet.com.com&NODEID=1104
New feature I like most "View Selection Source"

"Yes, tabs and all are nice, but to me the killer feature (beyond just the
general stability and standard's support) is the least noticed and most
unobtrusive new feature of Mozy 1.1.

Select any static text or image off the page, click-in the context menu, and
there at the bottom is "View Selection Source". Imagine being able to copy a
table or other HTML feature from one page to another, without having to hunt
through a lot of bulky markup for the particular snippet of HTML or XHTML or
MathML to transfer? Saves time, hooo-boy does it ever.

I don't recall seing the feature in IE or Opera. Is it there yet?"

    
    

    
  
Confirming. This feature is lovely :)))
QA Contact: pmac → sairuh

    
    

    
  
vrfy'd "view selection source" context menu item --tested with 2002.10.22.08
comm trunk builds.
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite

    
  
Component: XP Apps: GUI Features → UI Design

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: