Create HTML page containing all the elements necessary for L10N testing of the context menu access keys

RESOLVED FIXED

Status

Mozilla QA
Infrastructure
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Daniela Petrovici, Assigned: Daniela Petrovici)

Tracking

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

5 years ago
We need a HTML page on mozqa.com containing all the elements for testing the context menu access keys for l10n tests (based on bug 799149 - comment #15).

All the elements that need to be added to this test page are from the 29 cases added here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/test_contextmenu.html (per comment #6 in the same bug).
(Assignee)

Updated

5 years ago
Blocks: 799149
(Assignee)

Comment 1

5 years ago
Created attachment 738521 [details] [diff] [review]
patch v1.0

I have found the page that is used for testing on mxr (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/subtst_contextmenu.html) and modified it a little to work for our tests. I also had to add the audio.ogg file found in the same location on mxr since we do not have a video file with only audio for testing.

Also, the following cases needed to be removed due to the fact that we are running tests against multiple languages and one word in a language can produce no suggestions, but can have one or two suggestions in another language. Due to this, the context menu will change based on language and the word used for testing.
    <input id="test-input-spellcheck" type="text" spellcheck="true"
           autofocus value=...>
    <textarea id="test-textarea" style="width: 200px; height: 200px;">
    <div id="test-contenteditable"
         contenteditable="true">
Attachment #738521 - Flags: feedback?(hskupin)
Attachment #738521 - Flags: feedback?(dave.hunt)
(Assignee)

Comment 2

5 years ago
Created attachment 738915 [details] [diff] [review]
patch v1.1

Please ignore the previous comment. Since we are testing access keys and the spelling suggestions shouldn't have access keys, the three cases do not depend on locale. So, I have added them to the page.
Assignee: nobody → dpetrovici
Attachment #738521 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #738521 - Flags: feedback?(hskupin)
Attachment #738521 - Flags: feedback?(dave.hunt)
Attachment #738915 - Flags: feedback?(hskupin)
Attachment #738915 - Flags: feedback?(dave.hunt)
Comment on attachment 738915 [details] [diff] [review]
patch v1.1

Review of attachment 738915 [details] [diff] [review]:
-----------------------------------------------------------------

In general we should make this page more user friendly especially when it has to be located on mozqa.com too. Would you mind to add sections for kind of controls for a better overview? The page as is is for pure automation. Some more comments you can find inline.

I think it's the right direction but more changes have to be made. f+.

::: firefox/layout/contextMenuItems.html
@@ +7,5 @@
> +    <br>
> +    <div id="test-text">Lorem ipsum dolor sit amet, consectetuer adipiscing elit.</div>
> +    <div id="test-select-text">Lorem ipsum dolor sit amet, consectetuer adipiscing elit.</div>
> +    <div id="test-select-text-link">http://mozilla.com</div>
> +    <div id="test-dom-full-screen">Test for DOM Full Screen</div>

I don't see the difference in those divs. Can you please explain?

@@ +15,5 @@
> +    <textarea id="test-textarea">testTextArea</textarea>
> +    <div id="test-contenteditable" contenteditable="true">testContentEditable</div>
> +    <div contextmenu="myMenu">
> +      <p id="test-pagemenu" hopeless="true">Page Menu</p>
> +      <menu id="myMenu" type="context">

We want to test the context menu of Firefox and not HTML5 menus. So I don't see why we need this section. Can you please explain?

@@ +99,5 @@
> +    <iframe id="test-image-in-iframe" src="../images/mozilla_logo.jpg"
> +            width="98" height="98" style="border: 1px solid black">
> +    </iframe>
> +    <br>
> +    <embed id="test-plugin" style="width: 200px; height: 200px;" type="application/x-test"></embed>

Beside embed I miss an object tag to test. Should we reference real content?

::: firefox/video/audio.ogg
@@ +1,1 @@
> +<!DOCTYPE html>

This is not an ogg file. Further I don't see for which case we need it. Can you please explain?
Attachment #738915 - Flags: feedback?(hskupin)
Attachment #738915 - Flags: feedback?(dave.hunt)
Attachment #738915 - Flags: feedback+
(Assignee)

Comment 4

5 years ago
Created attachment 742993 [details] [diff] [review]
patch v1.2

(In reply to Henrik Skupin (:whimboo) from comment #3)
> ::: firefox/layout/contextMenuItems.html
> @@ +7,5 @@
> > +    <div id="test-dom-full-screen">Test for DOM Full Screen</div>
> I don't see the difference in those divs. Can you please explain?
I have used all the cases that are added on mxr, but since we are testing the context menu access keys only, I don't think we need all the different divs. We only need the one for plain text and the one for text content editable since their context menu is different. 
The one for the link is also different, but to get it to open we need to select the whole link which does not work with double click. Double click will only select a word from the link which does not help.

> @@ +15,5 @@
> > +      <menu id="myMenu" type="context">
> We want to test the context menu of Firefox and not HTML5 menus. So I don't
> see why we need this section. Can you please explain?
I have added it because it had other options in the context menu, but since the access keys are for the same options as the plain text, we don't need it. I have removed it from the page.

> @@ +99,5 @@
> > +    <embed id="test-plugin" style="width: 200px; height: 200px;" type="application/x-test"></embed>
> Beside embed I miss an object tag to test. Should we reference real content?
I have referenced real content and added the object tag.
> ::: firefox/video/audio.ogg
> This is not an ogg file. Further I don't see for which case we need it. Can
> you please explain?
This is for testing the context menu for a video that is invalid which is different from video with invalid media source. It is working properly with an empty file though, so I have removed all the content from it.
Attachment #738915 - Attachment is obsolete: true
Attachment #742993 - Flags: review?(andreea.matei)
(In reply to Daniela Petrovici from comment #4)
> Created attachment 742993 [details] [diff] [review]
> patch v1.2
> 
> (In reply to Henrik Skupin (:whimboo) from comment #3)
> The one for the link is also different, but to get it to open we need to
> select the whole link which does not work with double click. Double click
> will only select a word from the link which does not help.

You don't want a dblclick here but a simple right click. Not all the elements require that you make a selection first.
(Assignee)

Comment 6

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #5)
> (In reply to Daniela Petrovici from comment #4)
> > Created attachment 742993 [details] [diff] [review]
> > patch v1.2
> > 
> > (In reply to Henrik Skupin (:whimboo) from comment #3)
> > The one for the link is also different, but to get it to open we need to
> > select the whole link which does not work with double click. Double click
> > will only select a word from the link which does not help.
> 
> You don't want a dblclick here but a simple right click. Not all the
> elements require that you make a selection first.
The right click works properly without selecting the item first only for link created with "a" tag. For the above case we have a link written in a "div" tag and not selecting the whole link first does not open the correct context menu
Oh, misread then. In such a case you might want to manually create a selection for the specified text, or put it into a single line and trigger a triple-click.
Comment on attachment 742993 [details] [diff] [review]
patch v1.2

Review of attachment 742993 [details] [diff] [review]:
-----------------------------------------------------------------

Overall I don't think we have the 80 characters restrictions over html pages, so we can indent and close tags accordingly, either at the end of the line or under the opening one. It's cleaner and easier to follow.

::: firefox/layout/contextMenuItems.html
@@ +25,5 @@
> +    <img id="test-image" src="../images/mozilla_logo.jpg" width="50" height="50">
> +    <h4> Different types of links</h4>
> +    <table>
> +      <tr>
> +        <td><h5> Plain text link</h5></td>

You should remove the h5 tag so we can have the css applied here as well.

@@ +32,5 @@
> +      </tr>
> +      <tr>
> +        <td><a id="test-link" href="http://mozilla.com">ClickTheLink</a></td>
> +        <td><a id="test-mailto" href="mailto:noone@mozilla.com">MailToTink</a></td>
> +        <td><a id="test-image-link" href=""><img src="../images/mozilla_favicon.ico"></a>

I would move this image above to the images section, eventually use the one from there but add it a link. Title can be "Images - simple and linked"

@@ +55,5 @@
> +        <td>Canvas</td>
> +        <td>Video with valid media source</td>
> +        <td>Invalid video</td>
> +        <td>Video with invalid media source</td>
> +        <td>Video with wrong type</td>

I would suggest moving the canvas tag separate and have the 4 video types on 2 rows, it would look more aligned with the rest of the page.

@@ +59,5 @@
> +        <td>Video with wrong type</td>
> +      </tr>
> +      <tr>
> +        <td><canvas id="test-canvas" width="200" height="100"
> +                     style="background-color: blue">

background-color will not work without any context between tags.
Attachment #742993 - Flags: review?(andreea.matei) → review-
(Assignee)

Comment 9

5 years ago
Created attachment 747977 [details] [diff] [review]
patch v2.0

Updated patch based on review
Attachment #742993 - Attachment is obsolete: true
Attachment #747977 - Flags: review?(andreea.matei)
Comment on attachment 747977 [details] [diff] [review]
patch v2.0

Review of attachment 747977 [details] [diff] [review]:
-----------------------------------------------------------------

Looks way better now. Henrik, do you have any other requests here?
Attachment #747977 - Flags: review?(andreea.matei) → review+
Comment on attachment 747977 [details] [diff] [review]
patch v2.0

Review of attachment 747977 [details] [diff] [review]:
-----------------------------------------------------------------

Please flag for an additional review in the future.
Attachment #747977 - Flags: review?(hskupin)
Comment on attachment 747977 [details] [diff] [review]
patch v2.0

Review of attachment 747977 [details] [diff] [review]:
-----------------------------------------------------------------

Can you please rename the file to html_elements.html? It's content has nothing to do with context menu, and we might want to use the file for a variety of other tests too.

::: firefox/layout/contextMenuItems.html
@@ +6,5 @@
> +  <body>
> +    <style>
> +      table { border-collapse:collapse; }
> +      table td, table th { border: 1px solid black; padding: 5px; text-align: center; }
> +    </style>

style in the head tag please.

@@ +10,5 @@
> +    </style>
> +    <h2>Element needed in order to test the context menu</h2>
> +    <h4> Text area</h4>
> +    <textarea id="test-textarea" rows="1" cols="30">testTextArea</textarea>
> +    <h4>Canvas</h4>

Can you please add blank lines to separate different groups of elements?

@@ +13,5 @@
> +    <textarea id="test-textarea" rows="1" cols="30">testTextArea</textarea>
> +    <h4>Canvas</h4>
> +    <canvas id="test-canvas" width="200" height="100" style="background-color: turquoise"></canvas>
> +    <h4>Different types of inputs</h4>
> +    <input id="test-input-spellcheck" type="text" spellcheck="true" autofocus value="testInputWithSpellcheck">

All the input tags don't have a closing tag.

@@ +22,5 @@
> +    <br>
> +    <input id="test-input">
> +    <br>
> +    <h4>Images - simple and linked</h4>
> +    <table>

Why a table? I don't think that offers us anything. Can you please explain?

@@ +30,5 @@
> +      </tr>
> +      <tr>
> +        <td>
> +          <img id="test-image" src="../images/mozilla_logo.jpg" width="50" height="50">
> +        </td>

Make those single lines and also close the img tags.

@@ +60,5 @@
> +      </tr>
> +      <tr>
> +        <td>
> +          <video controls id="test-video-good"
> +                 src="../video/sample-video-10s-nosound.webm"

We want to have this test file local in the mozmill-tests repository later. So you cannot reference video files relatively.
Attachment #747977 - Flags: review?(hskupin) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 749882 [details] [diff] [review]
patch v2.1

I have made the changes per the last review.

> > +    <table>
> 
> Why a table? I don't think that offers us anything. Can you please explain?
I have used tables in order to make the layout of the page more user friendly. In case tables are not ok, would it suffice to just add elements separated by <br> tags, but the ordering to remain the same?
Attachment #747977 - Attachment is obsolete: true
Attachment #749882 - Flags: review?(hskupin)
Comment on attachment 749882 [details] [diff] [review]
patch v2.1

Review of attachment 749882 [details] [diff] [review]:
-----------------------------------------------------------------

Some nits. With those fixed we will be ok with the landing.

::: firefox/layout/html_elements.html
@@ +16,5 @@
> +    <canvas id="test-canvas" width="200" height="100" style="background-color: turquoise"></canvas>
> +
> +    <h4>Different types of inputs</h4>
> +    <input id="test-input-spellcheck" type="text" spellcheck="true" autofocus value="testInputWithSpellcheck"></input>
> +    <br>

nit: Every tag needs to be closed.

@@ +25,5 @@
> +    <input id="test-input"></input>
> +    <br>
> +
> +    <h4>Images - simple and linked</h4>
> +    <table>

Tables are usually not used for layout. But I wouldn't blame about it as of now. If you want to replace them with divs please do so.

@@ +31,5 @@
> +        <td>Simple image</td>
> +        <td>Linked image</td>
> +      </tr>
> +      <tr>
> +        <td><img id="test-image" src="../images/mozilla_logo.jpg" width="50" height="50"></img></td>

No, closing an image tag should work like <img ... />. A couple of others still miss it completely.
Attachment #749882 - Flags: review?(hskupin) → review-
(Assignee)

Comment 15

5 years ago
Created attachment 751645 [details] [diff] [review]
patch v2.2

(In reply to Henrik Skupin (:whimboo) from comment #14)
> Comment on attachment 749882 [details] [diff] [review]
> Tables are usually not used for layout. But I wouldn't blame about it as of
> now. If you want to replace them with divs please do so.

I tried using divs, but the layout looked like all elements were on one column or side by side but without spaces between them. So, I left the usage of table instead of div.
Attachment #749882 - Attachment is obsolete: true
Attachment #751645 - Flags: review?(hskupin)
Attachment #751645 - Flags: review?(dave.hunt)
Comment on attachment 751645 [details] [diff] [review]
patch v2.2

Review of attachment 751645 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I missed one thing in the last review. Just noticed it now during testing the page. With that fixed we should be good to go.

::: firefox/layout/html_elements.html
@@ +138,5 @@
> +      </tr>
> +      <tr>
> +        <td>
> +          <embed id="test-plugin" src="http://mozqa.com/data/firefox/plugins/flash/sample-flv-video-10s.flv"
> +                 style="width: 200px; height: 200px;">

Please use videos without sound. In that case there is an equivalent one in the same folder. Do we further need auto-play?
Attachment #751645 - Flags: review?(hskupin)
Attachment #751645 - Flags: review?(dave.hunt)
Attachment #751645 - Flags: review-
(Assignee)

Comment 17

5 years ago
Created attachment 752582 [details] [diff] [review]
patch v2.3

Changed the video to be the one without sound
Attachment #751645 - Attachment is obsolete: true
Attachment #752582 - Flags: review?(hskupin)
Attachment #752582 - Flags: review?(dave.hunt)
I'm still waiting for an answer to my question regarding auto-play. Do we need that?
(Assignee)

Comment 19

5 years ago
Created attachment 752773 [details] [diff] [review]
patch v2.4

(In reply to Henrik Skupin (:whimboo) from comment #18)
> I'm still waiting for an answer to my question regarding auto-play. Do we
> need that?
We don't need the auto-play since we are testing the context menu access keys and the additional option ("Open in Movie Player") does not seem to have one. Removed autoplay from the video that is embedded and also found an issue with the iframe video which had a wrong path.

I could not remove the autoplay from the iframe video. I tried with {video_URL}?autoplay=0 (or =false), but it seems that this option is not taken into account.
Attachment #752582 - Attachment is obsolete: true
Attachment #752582 - Flags: review?(hskupin)
Attachment #752582 - Flags: review?(dave.hunt)
Attachment #752773 - Flags: review?(hskupin)
Attachment #752773 - Flags: review?(dave.hunt)
Comment on attachment 752773 [details] [diff] [review]
patch v2.4

Review of attachment 752773 [details] [diff] [review]:
-----------------------------------------------------------------

I would suggest not to upload new patches if there are still outstanding issues. Wait until the questions have been solved. Uploading new patches en mass doesn't really help us.

::: firefox/layout/html_elements.html
@@ +117,5 @@
> +          </iframe>
> +        </td>
> +        <td>
> +          <iframe id="test-video-in-iframe"
> +                  src="http://mozqa.com/data/firefox/video/sample-video-10s-nosound.webm"

Usually you specify HTML pages within the src attribute. Those can contain images and other content. Shouldn't we specify one of those pages we have under the video folder? That might also fix the auto play issue.
Attachment #752773 - Flags: review?(hskupin)
Attachment #752773 - Flags: review?(dave.hunt)
(Assignee)

Comment 21

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #20)
> Comment on attachment 752773 [details] [diff] [review]
> patch v2.4
> 
> Review of attachment 752773 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would suggest not to upload new patches if there are still outstanding
> issues. Wait until the questions have been solved. Uploading new patches en
> mass doesn't really help us.
> 
> ::: firefox/layout/html_elements.html
> @@ +117,5 @@
> > +          </iframe>
> > +        </td>
> > +        <td>
> > +          <iframe id="test-video-in-iframe"
> > +                  src="http://mozqa.com/data/firefox/video/sample-video-10s-nosound.webm"
> 
> Usually you specify HTML pages within the src attribute. Those can contain
> images and other content. Shouldn't we specify one of those pages we have
> under the video folder? That might also fix the auto play issue.

If we specify a HTML page in the src attribute we will not have the same context menu as for a video added directly in the src. The context menu will be for a page, whereas the context menu for video has Play, Pause, Mute etc. controls.
(Assignee)

Comment 22

5 years ago
Created attachment 753742 [details] [diff] [review]
patch v2.5

> Usually you specify HTML pages within the src attribute. Those can contain
> images and other content. Shouldn't we specify one of those pages we have
> under the video folder? That might also fix the auto play issue.
You were right about using HTML pages, the only issue when I tested was that using a page from video folder did not help. The page needs to contain only the video in order for the correct context menu to be displayed. So, I have created a HTML page with only video and this is working properly now and the video is not auto-played.
Attachment #752773 - Attachment is obsolete: true
Attachment #753742 - Flags: review?(hskupin)
Dependent on the content the context menu is shown. So if you have a page which has a video inside but you right click outside of the video, you will not get the entries you are expecting to see. Right now I can see that we have used the src=video link in http://mozqa.com/data/firefox/plugins/flash/test_swf_iframes_nosound.html too. So probably we should do it the same way here and re-think what do do later. Can you please restore the former version of the patch? We can get this landed then.
(Assignee)

Comment 24

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #23)
> Can you please restore the former version of the patch? We can get
> this landed then.
The previous patch contained the sample-video-10s-nosound.webm URL in the iframe. I could not set the auto-play=false for it. Based on the discussion we had on IRC, my understanding is that I should leave this patch as it is.
Comment on attachment 753742 [details] [diff] [review]
patch v2.5

Review of attachment 753742 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/video/test_video_in_iFrame_nosound.html
@@ +1,1 @@
> +<!DOCTYPE html>

The problem you are facing is that the table cell has too less space to show the video when you reference an existing html file. All of them have a big heading above the video. So you could scroll the video element into view first, or we really create a new test file. For the latter option you should rename this file to something like test_ogv_video_nosound_plain.html.

Can you please check if option 1 works? I don't think we should introduce new test files when we really can re-use existing ones. That only adds maintenance work for us.
Attachment #753742 - Flags: review?(hskupin)
(Assignee)

Comment 26

5 years ago
Created attachment 754759 [details] [diff] [review]
patch v2.6

(In reply to Henrik Skupin (:whimboo) from comment #25)
> Can you please check if option 1 works? 
It works. When I am using scrollIntoView for the movie element inside the HTML page from iFrame, the correct context menu is displayed.
Attachment #753742 - Attachment is obsolete: true
Attachment #754759 - Flags: review?(hskupin)
Comment on attachment 754759 [details] [diff] [review]
patch v2.6

Review of attachment 754759 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/layout/html_elements.html
@@ +5,5 @@
> +    <style>
> +      table { border-collapse: collapse; }
> +      table td, table th { border: 1px solid black; padding: 5px; text-align: center; }
> +    </style>
> +  </head>

So the only remaining entry here is the encoding of the file. Let me add this and push it to the repository.
Attachment #754759 - Flags: review?(hskupin) → review+
This patch did not contain a commit message. So I have added this. Next time please remember that.

Landed as:
http://hg.mozilla.org/qa/testcase-data/rev/8cbb79622320
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.