Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Filepicker
    • Labels:
    • Testing Instructions:
      Hide
      1. Login as admin
      2. Enable Google docs repository (NOTE: use localhost site name as moodle.local won't work)
      3. Access private files and open file picker
      4. Click on Google docs repository
      5. On top select seach box and make sure text get selected.
      6. Type some key word and hit enter and make sure appropriate search results are shown
      7. Repeat above 2 steps and make sure Search text is selected and nothing is wrong.
      Show
      Login as admin Enable Google docs repository (NOTE: use localhost site name as moodle.local won't work) Access private files and open file picker Click on Google docs repository On top select seach box and make sure text get selected. Type some key word and hit enter and make sure appropriate search results are shown Repeat above 2 steps and make sure Search text is selected and nothing is wrong.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-mdl-33506
    • Rank:
      41409

      Description

      In the file picker, the search box has the word 'Search' in it.

      In order to do a search you need to delete that text. In similar interfaces like this the text is automatically highlighted.

        Activity

        Hide
        Michael de Raadt added a comment -

        Can we not use the placeholder attribute? That seems to work on all browsers except IE.

        I'm not sure if this is something that must be fixed before 2.3. I'll leave that up to you, Marina.

        Show
        Michael de Raadt added a comment - Can we not use the placeholder attribute? That seems to work on all browsers except IE. I'm not sure if this is something that must be fixed before 2.3. I'll leave that up to you, Marina.
        Hide
        Dan Poltawski added a comment -

        Looks and works perfectly. Thanks Raj

        Show
        Dan Poltawski added a comment - Looks and works perfectly. Thanks Raj
        Hide
        Rajesh Taneja added a comment -

        Thanks for review Dan
        Pushing for integration review.

        Show
        Rajesh Taneja added a comment - Thanks for review Dan Pushing for integration review.
        Hide
        Sam Hemelryk added a comment -

        Hi guys,

        Failing this for two reasons presently.

        1. This patch highlights for every click, not just the first, and despite the content of the search. As such if you type the following search: 'I am a comlpex search' you cannot click back to the middle of the complex to fix the typo.
        2. Repositories can override the content of that search form. There's nothing to say the the search input will be the first input element (consider hidden inputs) or in fact nothing to say that there will be an input at all which will result in a JS error.

        Presently I think the following needs to happen before this can be integrated.

        1. Proper logic created to highlight the contents of the search box in the desired circumstances. We only want it to highlight if the default contents are there, and only on the first click I imagine (so you can click one to enter the box and a second time to get a cursor within the default search).
        2. A more pragmatic approach needs to be used in finding the node to attach any events to, we know the form is always going to be there, however we can not be sure about any inputs. We do know that any search input will have name="s" so we can work with that I suppose. Also nothing to say it will have content so check it has !empty content before attaching anything.

        Anyway feel free to ping me to discuss if you want.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi guys, Failing this for two reasons presently. This patch highlights for every click, not just the first, and despite the content of the search. As such if you type the following search: 'I am a comlpex search' you cannot click back to the middle of the complex to fix the typo. Repositories can override the content of that search form. There's nothing to say the the search input will be the first input element (consider hidden inputs) or in fact nothing to say that there will be an input at all which will result in a JS error. Presently I think the following needs to happen before this can be integrated. Proper logic created to highlight the contents of the search box in the desired circumstances. We only want it to highlight if the default contents are there, and only on the first click I imagine (so you can click one to enter the box and a second time to get a cursor within the default search). A more pragmatic approach needs to be used in finding the node to attach any events to, we know the form is always going to be there, however we can not be sure about any inputs. We do know that any search input will have name="s" so we can work with that I suppose. Also nothing to say it will have content so check it has !empty content before attaching anything. Anyway feel free to ping me to discuss if you want. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Oh btw, is this really something we need to fix before 2.3? Personally I don't think this is very important or is a requirement to the 2.3 release.
        My +1 to remove the MUST FIX FOR 2.3.

        Show
        Sam Hemelryk added a comment - Oh btw, is this really something we need to fix before 2.3? Personally I don't think this is very important or is a requirement to the 2.3 release. My +1 to remove the MUST FIX FOR 2.3.
        Hide
        Dan Poltawski added a comment -

        Bah, I should've spotted those issues during peer review.

        Anway I agree about taking this out of the MUST FIX backlog and so have done that. Particularly because its not widely used across repositories (I thought it was visible on every repo when I first created this issue).

        Show
        Dan Poltawski added a comment - Bah, I should've spotted those issues during peer review. Anway I agree about taking this out of the MUST FIX backlog and so have done that. Particularly because its not widely used across repositories (I thought it was visible on every repo when I first created this issue).
        Hide
        Rajesh Taneja added a comment - - edited

        Thanks for spotting the problem Sam.
        Patch has been rectified, taking care of editing (click/drag) search text.

        Show
        Rajesh Taneja added a comment - - edited Thanks for spotting the problem Sam. Patch has been rectified, taking care of editing (click/drag) search text.
        Hide
        Rajesh Taneja added a comment - - edited

        Hello Dan,

        I have added another patch and tested on IE, FF, Chrome, and Opera. In chrome this.select() doesn't work and it's a known issue (http://code.google.com/p/chromium/issues/detail?id=4505)

        Best workaround is to select text after timeout, Please suggest if any of the solutions is acceptable.

        Second patch:
        wip-mdl-33506-new
        https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-33506-new

        Show
        Rajesh Taneja added a comment - - edited Hello Dan, I have added another patch and tested on IE, FF, Chrome, and Opera. In chrome this.select() doesn't work and it's a known issue ( http://code.google.com/p/chromium/issues/detail?id=4505 ) Best workaround is to select text after timeout, Please suggest if any of the solutions is acceptable. Second patch: wip-mdl-33506-new https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-33506-new
        Hide
        Tim Hunt added a comment -

        I don't think we should hack around Chrome bugs, not if the hack is something ugly like settimeout. So, I think

        https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-33506

        is better than

        https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-33506-new

        Show
        Tim Hunt added a comment - I don't think we should hack around Chrome bugs, not if the hack is something ugly like settimeout. So, I think https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-33506 is better than https://github.com/rajeshtaneja/moodle/compare/master...wip-mdl-33506-new
        Hide
        Rajesh Taneja added a comment -

        Thanks Tim,

        I agree with you. I just put second patch because correct behaviour is visible with focus event. Will wait for Dan's input and then push it back for integration.

        Show
        Rajesh Taneja added a comment - Thanks Tim, I agree with you. I just put second patch because correct behaviour is visible with focus event. Will wait for Dan's input and then push it back for integration.
        Hide
        Dan Poltawski added a comment -

        Yep I agere with Tim

        Show
        Dan Poltawski added a comment - Yep I agere with Tim
        Hide
        Rajesh Taneja added a comment -

        Thanks Tim, Dan and Sam.

        Pushing it back for integration.

        Show
        Rajesh Taneja added a comment - Thanks Tim, Dan and Sam. Pushing it back for integration.
        Hide
        Sam Hemelryk added a comment -

        OK thanks Raj, this has been integrated now.
        It appears to work pretty consistently, the only thing I noted was that a click and drag highlight of partial content would be lost because of the click event.
        However the search box doesn't remember the last search and I can't imagine too many people highlighting part of "Search" so in it goes.

        Show
        Sam Hemelryk added a comment - OK thanks Raj, this has been integrated now. It appears to work pretty consistently, the only thing I noted was that a click and drag highlight of partial content would be lost because of the click event. However the search box doesn't remember the last search and I can't imagine too many people highlighting part of "Search" so in it goes.
        Hide
        Sam Hemelryk added a comment -

        Tested cross browser during integration review

        Show
        Sam Hemelryk added a comment - Tested cross browser during integration review
        Hide
        Eloy Lafuente (stronk7) added a comment -

        And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay!

        Many, many thanks for your hard work!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay! Many, many thanks for your hard work! Ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: