587617 - Can't select text in the output of the WebConsole on linux

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

With the integration of the patch from bug 586514, the CSS styling that enabled text selection inside the output node of the WebConsole no longer works. Users cannot select text inside the console after switching to XUL.

We need to fix that bit of styling code.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: fix + test code

This is the fix for the Linux-specific stylesheet. In the XULification bug this change was made only for the Mac and Windows styles.

The test code now makes sure that output nodes selection, menu Edit > Copy item and the actual selection copy command all work fine. Thus, this patch fixes bug 586386 as well, making the test code from bug 574036 obsolete. Once reviewed (or earlier?), we should send this patch to the try server, so we make sure it is not failing on other systems (wfm on Linux).

Thanks!

Comment 3

[Mihai Sucan [:msucan]]

Comment on attachment 466317 [details] [diff] [review]
fix + test code

Thanks David for the feedback+!

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 466317 [details] [diff] [review]
fix + test code

>         text = null;
>-        testWebConsoleClose();
>+        testOutputCopy();
>       });
>     }

why removing the other function call?

>-.hud-output-node div {
>+.hud-output-node * {

would prefer more granularity if possible.

r=me w/ these answered/fixed.

Comment 5

[Mihai Sucan [:msucan]]

Thank you Dietrich for the review!

(In reply to comment #4)
> Comment on attachment 466317 [details] [diff] [review]
> fix + test code
> 
> >         text = null;
> >-        testWebConsoleClose();
> >+        testOutputCopy();
> >       });
> >     }
> 
> why removing the other function call?

Many of the tests in the HUDService tests file are async tests that need to be chained from the previous one. You never know when one starts or one finishes.

This situation is not ideal, but we will have a fix for this issue in a bug patch that will split the HUDService tests into multiple files.


> >-.hud-output-node div {
> >+.hud-output-node * {
> 
> would prefer more granularity if possible.

We have multiple types of elements in .hud-output-node, and this is what Patrick has also done for the Windows and MacOS stylesheets. I just "ported" the change to the Linux style.

Comment 6

[David Dahl :ddahl]

test failure:

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | Timed out while polling clipboard for pasted data. Expected 15:21:45:234: Hello world!
make: *** [mochitest-browser-chrome] Error 1

Comment 7

[David Dahl :ddahl]

So this test fails on linux, but the patch indeed works. I tried running it 2 or 3 times and it failed each time.

Comment 8

[Mihai Sucan [:msucan]]

Attachment: rebased patch

David: You are correct, the patch applies cleanly, and the test code fails. Still, it only needed a rebase, because mozilla-central has checked-in the WebConsole output filtering code, which left the output in an incorrect state for the test code I have.

The fix was minor: clear the filterBox.value.

Thanks!

Comment 9

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 468045 [details] [diff] [review]
rebased patch

http://hg.mozilla.org/mozilla-central/rev/6d8a749a3c57

Comment 10

[Rob Campbell [:rc] (:robcee)]

Reopening.

Backed out in revision 87677ea8efa3

Comment 11

[Mihai Sucan [:msucan]]

Comment on attachment 468045 [details] [diff] [review]
rebased patch

Marco: we have a problem with this test. It fails on mozilla-central default branch. Could you please take a look and let us know if you have any ideas why this fails? We get a timeout, but there's no obvious error.

On my system I cannot reproduce the issue. All tests always pass.

Thank you very much!

Comment 12

[Marco Bonardo [:mak]]

the problem is most likely in selectedNode.focus(), indeed if you comment out that focusing you get same failure on Windows as well. We always had focus issues on Linux, where focus is usually more lazy, that's why we had to create waitForFocus. Since you fail only on Linux it's probably related to that.

Comment 13

[Mihai Sucan [:msucan]]

Attachment: updated patch

Marco: thanks for your quick reply!

I updated the patch to use waitForFocus(). The test still passes on my system (I'm on Linux, btw). The test failed on Mac OS X when it was checked-in.

I would note that the test did not reach the selectedNode.focus() part. It did not even get to the execution of the make_selection() event handler. Problem is around here:

  menu_popup.addEventListener("popupshown", function (aEvent) {
    if (aEvent.target == menu_popup) {
      aEvent.target.removeEventListener(aEvent.type, arguments.callee, false);
      make_selection();
    }
  }, false);
  menu_popup.openPopup(null, "overlap", 0, 0, false, false);

It looks like the event handler never fired. I don't know why would this fail on Mac and work on Linux.

Comment 14

[Mihai Sucan [:msucan]]

Neil: do you know a reason why a menupopup can fail to show on Mac OS X?

The test code does:

var menu_popup = document.getElementById("menu_EditPopup");
// menu_EditPopup is in browser-menubar.inc.

menu_popup.addEventListener("popupshown", function (aEvent) {
  if (aEvent.target == menu_popup) {
    aEvent.target.removeEventListener(aEvent.type, arguments.callee, false);
    make_selection();
  }
}, false);
menu_popup.openPopup(null, "overlap", 0, 0, false, false);

The event handler is never fired on Macs. I can only test on Linux, and it works fine for me - it never fails.

(for more context, please see attachment 468685 [details] [diff] [review].)

Thank you very much! Sorry for bothering you so much with these XUL matters.

Comment 15

[Neil Deakin]

someNode.focus() is always synchronous on all platforms. waitForFocus should only be used when a focus() call is desired on a window.


(In reply to comment #14)
> menu_popup.addEventListener("popupshown", function (aEvent) {
>   if (aEvent.target == menu_popup) {
>     aEvent.target.removeEventListener(aEvent.type, arguments.callee, false);
>     make_selection();
>   }
> }, false);
> menu_popup.openPopup(null, "overlap", 0, 0, false, false);

As an aside, "after_start" is the way menus are opened, not "overlap". Or, just use menu_popup.parentNode.open = true;


> 
> The event handler is never fired on Macs. I can only test on Linux, and it
> works fine for me - it never fails.

The Mac uses a native menubar, so attempting to open the non-native popup won't do anything, as it doesn't actually exist. I can't tell from the test what it's actually testing for, so I can't answer what should be used instead, if anything.

Comment 16

[Mihai Sucan [:msucan]]

(In reply to comment #15)
> someNode.focus() is always synchronous on all platforms. waitForFocus should
> only be used when a focus() call is desired on a window.

Thanks. I had a hunch about that. Then I cam remove the waitForFocus() call, correct?


> (In reply to comment #14)
> > menu_popup.addEventListener("popupshown", function (aEvent) {
> >   if (aEvent.target == menu_popup) {
> >     aEvent.target.removeEventListener(aEvent.type, arguments.callee, false);
> >     make_selection();
> >   }
> > }, false);
> > menu_popup.openPopup(null, "overlap", 0, 0, false, false);
> 
> As an aside, "after_start" is the way menus are opened, not "overlap". Or, just
> use menu_popup.parentNode.open = true;

Cool, thanks for the tip.


> > The event handler is never fired on Macs. I can only test on Linux, and it
> > works fine for me - it never fails.
> 
> The Mac uses a native menubar, so attempting to open the non-native popup won't
> do anything, as it doesn't actually exist. I can't tell from the test what it's
> actually testing for, so I can't answer what should be used instead, if
> anything.

Yay for platform ownage, hehe. :)

Here's what the test tries to do:

1. select some text in the WebConsole output.
2. see if Edit > Copy is enabled.
3. invoke the cmd_copy and see if the selected node text was copied to clipboard.

The edit menu has a onpopupshowing event handler (updateEditUIVisibility()) which does cool stuff I want. I wanted to call that directly without going through the edit menupopup opening, but I think there's code that prevents changed to the Copy menuitem when the menu is not showing (or something).

Therefore, on my system (Linux) I got it all working by opening the edit menupopup.

Better ideas are very much welcome. I'd like to get this test to work cross-platform without failures. Thank you very much for your answers!

Comment 17

[Neil Deakin]

I'd just skip that part of the test on Mac. Or ask Josh about how to test it.

You could also check the <command id="cmd_copy"> to see if it is disabled, which will be set when the copy command is disabled.

Comment 18

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 19

[Mihai Sucan [:msucan]]

Attachment: updated patch

Thanks Neil for your answer!

I have rebased and updated the patch to no longer check the menu_copy item, just the cmd_copy command.

Comment 20

[Mihai Sucan [:msucan]]

Comment on attachment 472377 [details] [diff] [review]
updated patch

Asking for feedback on the test changes.

Comment 21

[Kevin Dangoor]

just a note that this bug is open right now for test changes only.

Comment 22

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch. I also moved the test code into a new file, so we don't cause problems to Patrick's test split work. Additionally, this morning I pushed the patch to the tryserver - no failures, no memleaks. Yay! I think the test code is finally right. :)

Comment 23

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6479b65705e1