1047215 - [Gallery/Video]Background Overlays are not translated

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

[vsireesha246]

STR:

Precondition:
Settings->USB storage->Enable

1.Open Gallery/Video app->It shows "Memory card in use" overlay
2.Press Home button to make it in background
3.Go to Settings->Languages->Change to any other language->click on OK button->Press/hold Home button
4.Open the Gallery/Video background overlay and click on it..the text is not translated.

Note:Same issue is not happening in Music app.

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

Yes, you are right. We haven't take this as a consideration. I cc Russ, another video peer, in.

Comment 2

[Punam Dahiya [:pdahiya]]

This issue should be resolved with the fix of Bug 1038984 for gallery and video app. Please try the patch attached to 1038984. Marking the bug duplicate of bug 1038984.

Comment 3

[gcharles]

Unfortunately, the video app does not use the same code as the gallery app for overlays so I am reopening this.

This could now be resolved by modifying the video app to use the shared/Dialogs.js `showOverlay` code. 
As outlined in the comments in https://github.com/mozilla-b2g/gaia/pull/21885.

Comment 4

[Russ Nicoletti [:russn]]

Attachment: PR -- https://github.com/mozilla-b2g/gaia/pull/22938

This PR includes the patch from bug 1038984 as those changes to shared/js/dialogs.js are required for this patch.

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8473900 [details] [review]
PR -- https://github.com/mozilla-b2g/gaia/pull/22938

Looks good. I left some comments at PR. BTW, I am sure if we need to change the l10n key in this case.

Comment 6

[vsireesha246]

Hi Russ,

https://github.com/mozilla-b2g/gaia/pull/22938

I found in this PR below problems.

1.Empty videos overlay is not shown(Without video in sdcard and internal memory).For this we need to add emptyvideo case in dialog.js

2.In dialog.js 

var textOption = options[prefix] || options[prefix + 'Text'];
should be replaced by
var textOption = options[prefix] || options[prefix + 'Id'];

Comment 7

[vsireesha246]

Hi Russ,

One more point i missed..we need to lazy load the confirm.css in index.html of video app.

Please let me know your comments,if am wrong.

Thanks..
Sireesha



(In reply to vsireesha246 from comment #6)
> Hi Russ,
> 
> https://github.com/mozilla-b2g/gaia/pull/22938
> 
> I found in this PR below problems.
> 
> 1.Empty videos overlay is not shown(Without video in sdcard and internal
> memory).For this we need to add emptyvideo case in dialog.js
> 
> 2.In dialog.js 
> 
> var textOption = options[prefix] || options[prefix + 'Text'];
> should be replaced by
> var textOption = options[prefix] || options[prefix + 'Id'];

Comment 8

[Punam Dahiya [:pdahiya]]

Comment on attachment 8473900 [details] [review]
PR -- https://github.com/mozilla-b2g/gaia/pull/22938

showOverlay method inside shared/js/dialogs.js is having too many app-specific localization ids and should be brought back inside respective app. Created bug 1055715 to remove showOverlay from shared/js/dialogs.js. 

As discussed over IRC, cancelling the review as the attached patch should be updated to remove showOverlay dependency for video on shared/js/dialogs.js

Comment 9

[Russ Nicoletti [:russn]]

I will investigate the issues in comment 6 and comment 7 when I refactor the original showOverlay in video.js per comment 8

Comment 10

[Russ Nicoletti [:russn]]

I have updated the PR so that the video app no longer has a dependency on shared/js/dialogs. Also, the video.js showOverlay now sets the 'data-l10n-id' attribute instead of 'textContent' to support dynamic language change.

The updated PR addresses the issues in comment 7.

Comment 11

[Russ Nicoletti [:russn]]

Correction: comment 10, "the video app no longer has a dependency on shared/js/dialogs" should be "the video app no longer has a dependency on shared/js/dialogs showOverlay"

Comment 12

[Russ Nicoletti [:russn]]

Giovanni, can you comment on John's questions regarding Dialogs.confirm here https://github.com/mozilla-b2g/gaia/pull/22938/files#r16393915 and here https://github.com/mozilla-b2g/gaia/pull/22938/files#r16394018? 

Btw, I copied shared/js/dialogs.js from your bug 1038984 patch as I needed those changes for this bug.

Comment 13

[Russ Nicoletti [:russn]]

Comment on attachment 8473900 [details] [review]
PR -- https://github.com/mozilla-b2g/gaia/pull/22938

John, I'm requesting another review because the fix has changed from using shared/js/dialogs showOverlay to showOverlay in the video app.

Comment 14

[Russ Nicoletti [:russn]]

Giovanni, here are the proper links to John's comments:

https://github.com/mozilla-b2g/gaia/pull/22938/files#r16394018
https://github.com/mozilla-b2g/gaia/pull/22938/files#r16393915

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

Punam is correct. The showOverlay depends on gallery's id heavier. I should find it at the previous patch review. That's my fault.

As to this version, it looks good for me, except the one that you are asking to Giovanni. We should wait for his answer before landing this. So, I keep the review flag.

Comment 16

[gcharles]

Comment added, https://github.com/mozilla-b2g/gaia/pull/22938/files#diff-c3e14d59821cfbb27fe47029d88349f6R22

Comment 17

[Russ Nicoletti [:russn]]

Giovanni, apologies for the broken links into the PR. If you search for 'giovannic' in the "files changed" tab of the PR you will find John's questions about the 'confirm' method in dialogs.js.

Comment 18

[gcharles]

My comment should be the most recent. I will quote it here:

"yes this function assumes the cases are mutually exclusive. The else-if version would be safer and clearer although they should not alter the behaviour of the function given the assumptions.

Thanks for the tip :)"

I have added the suggested changes to my patch.

Comment 19

[gcharles]

Something that is not mentioned in the comments is that the 2nd change proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=1047215#c6 is incorrect. 

The case shown here, https://github.com/mozilla-b2g/gaia/pull/22938/files#diff-c3e14d59821cfbb27fe47029d88349f6R31, was put in to preserve the existing behaviour of Dialogs.confirm as requested in https://bugzilla.mozilla.org/show_bug.cgi?id=1038984#c12.

Changing that line would make nonsensical behaviour.

Comment 20

[Russ Nicoletti [:russn]]

I have updated the PR for this bug with Giovanni's latest shared/js/dialogs.js patch. I also tested the gallery and video apps with this latest patch and they work as expected.

John, can you update the review flag? Thanks.

Comment 21

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8473900 [details] [review]
PR -- https://github.com/mozilla-b2g/gaia/pull/22938

Looks good to me. Please remove the console.log before landing the code. BTW, I found this patch is conflicted with others in index.html. You should rebase the code and check if you break any gaia-try. Make sure they are all passed before landing. Thanks.

Comment 22

[Russ Nicoletti [:russn]]

I have updated the patch because the semantics of showOverlay changed so the unit tests needed to be updated. The unit tests passed on the try server. However, when I run them locally, I'm getting global leak errors from mocha:

  1) [video-test/unit/video_test.js] Video App Unit Tests "before all" hook:
     Error: global leaks detected: LAYOUT_MODE, dom, ids, $, playing, playerShowing, endedTimer, controlShowing, controlFadeTimeout, selectedFileNames, selectedFileNamesToBlobs, currentVideo, currentVideoBlob, firstScanEnded, THUMBNAIL_WIDTH, THUMBNAIL_HEIGHT, HAVE_NOTHING, storageState, currentOverlay, dragging, touchStartID, isPausedWhileDragging, sliderRect, thumbnailList, pendingPick, restoreTime, isPhone, isPortrait, currentLayoutMode, pendingUpdateTitleText, FROMCAMERA, videoControlsAutoHidingMsOverride, init, initThumbnailSize, initLayout, initPlayerControls, initOptionsButtons, addEventListeners, toggleFullscreenPlayer, toggleVideoControls, handleScreenLayoutChange, switchLayout, handleActivityEvents, showInfoView, hideInfoView, showSelectView, hideSelectView, showOptionsView, hideOptionsView, clearSelection, updateSelection, launchCameraApp, resetCurrentVideo, deleteSelectedItems, deleteFile, shareSelectedItems, share, updateDialog, updateLoadingSpinner, thumbnailClickHandler, setPosterImage, showOverlay, setControlsVisibility, updateVideoControlSlider, setVideoPlaying, deleteCurrentVideo, handlePlayButtonClick, handleCloseButtonClick, postPickResult, shareCurrentVideo, handleSliderTouchStart, setVideoUrl, scheduleVideoControlsAutoHiding, setNFCSharing, showPlayer, hidePlayer, playerEnded, play, pause, timeUpdated, handleSliderTouchEnd, handleSliderTouchMove, toCamelCase, releaseVideo, restoreVideo, showPickView, cancelPick, cleanupPick, showThrobber, hideThrobber

I don't have an explanation for why I'm seeing these leaks after the recent changes and why they're passing on the try server. John, can you take a look? Thanks.

Comment 23

[John Hu [:johnhu][:johu][:醬糊小弟]]

Russ,

I cannot tell you why because I cannot reproduce it in my site. I had done `make really-clean` before apply your patch which could make sure everything is clean. I would suggest that you may `make really-clean` at master branch, apply your patch, and try again.

The main reason of global leaks is we set a variable to window which is not declared globally in this test file. The only one may be "LazyLoader". But I didn't get the error about it. The solution to fix this kind of error is declared it is globally, like:

mocha.globals(['LazyLoader', 'others', ...]);

It should be put before the first require().

The only one you need is LazyLoader, I think. But I cannot validate it because I cannot see the error or warning of it.

BTW, I found we haven't saw any global leaks recently. It may be disabled in test-agent with "--ignore-leaks".